Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Http object type #143

Closed
wants to merge 24 commits into from
Closed

Http object type #143

wants to merge 24 commits into from

Conversation

afek854
Copy link
Contributor

@afek854 afek854 commented Sep 2, 2024

Sorry, we do not accept changes directly against this repository. Please see
CONTRIBUTING.md for information on where and how to contribute instead.

Copy link

github-actions bot commented Sep 2, 2024

Summary:

  • License scan: success
  • Credentials scan: failure
  • Vulnerabilities scan: success
  • Unit test: success
  • Go linting: failure

@afek854 afek854 requested a review from matthyx September 5, 2024 09:20
Copy link
Contributor

@matthyx matthyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see more test coverage on the path analyser
and maybe some benchmarks too

pkg/apis/softwarecomposition/consts/consts.go Outdated Show resolved Hide resolved

type IsInternal string

const (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced we need to do this... this sounds non idiomatic Go to me - do you have some reference for this pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wdym by it? I did it because I could not transfer bool value (The bug we talked about)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn, forgot about it. Still looking ugly 🫣

SegmentName string
Count int
Children map[string]*SegmentNode
mutex sync.RWMutex
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused mutex

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONE

var newEndpoints []types.HTTPEndpoint
MergeDuplicateEndpoints(endpoints)
for _, endpoint := range *endpoints {
AnalyzeURL(endpoint.Endpoint, analyzer)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't use return value and error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In segment of the code no, because I would just skip errors.
We can talk about it

}
}

func shallowChildrensCopy(src, dst *SegmentNode) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

children is already plural

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONE


func AnalyzeURL(urlString string, analyzer *PathAnalyzer) (string, error) {
if !strings.HasPrefix(urlString, "http://") && !strings.HasPrefix(urlString, "https://") {
urlString = "http://" + urlString
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we want to default to https?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not matter as it is used only for parsing parsedURL, err := url.Parse(urlString)


const dynamicIdentifier string = "<dynamic>"

const theshold = 100
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

threshold

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONE

@matthyx
Copy link
Contributor

matthyx commented Sep 6, 2024

I have started a new branch, rebased on my changes to main (some refactoring to use gRPC) so you might want to use this one instead: https://github.com/kubescape/storage/tree/http-object-type-mb
Let's sync on Monday.

@afek854 afek854 closed this Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants