-
Notifications
You must be signed in to change notification settings - Fork 164
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
🎁 Modscope tool #3513
🎁 Modscope tool #3513
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cardil: 10 warnings.
In response to this:
Which issue(s) this PR fixes:
Related knative/hack#220/lint
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
00049b0
to
ac07a18
Compare
Signed-off-by: Chris Suszyński <ksuszyns@redhat.com>
ac07a18
to
c2ee469
Compare
/lint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cardil: 0 warnings.
In response to this:
/lint
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cardil This PR is quite big. Can you give an overview which stuff you wrote and which stuff is imported from somewhere else? (and from where). There's a lot of stuff under tools/modscope, where does it come from? Just some notes about things that could make the review easier.
indent_size = 2 | ||
|
||
[{*.go,Makefile}] | ||
indent_style = tab |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this editorconfig be sent in a separate PR? how does it belong to this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could remove it, if you feel like it. It's not crucial. I just piggyback it here for simplicity.
Well. I think the PR description (together with linked issue) answers all of those questions you've asked. All the stuff in this PR is my original work. Some functions are only inspired by Golang implementation, as stated in PR description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good.
Are you planning on running this in a prow job? If so, you need to install the binary on the prow-tests image.
The plan is to execute this in hack scripts. Directly, via https://github.com/knative/hack/blob/6c301965af4c4df7178a54070ce6281797ec6b43/library.sh#L672 with something like: done < <(go_run knative.dev/test-infra/tools/modscope@latest ls) |
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
thank you
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cardil, upodroid The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Which issue(s) this PR fixes:
Related knative/hack#220
Why is this needed?
To properly fix knative/hack#220 we need a better tooling for resolving the list of modules listed in
go.work
, and the current module as well. This tool adds both, by porting the same code from Golang https://github.com/golang/go/blob/4c1ca42aa295d68b6172b7f49e4fb4fd14d86475/src/cmd/go/internal/modload/init.go#L1274-L1319 and using development version of golang.org/x/mod/modfile package.