Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
depsync: add xk6-depsync command #72
depsync: add xk6-depsync command #72
Changes from 6 commits
b2111f6
0be4201
f562cbf
8ae8675
6bf8353
1a15a46
87a8947
c71ad22
622fc4f
c9cc4ff
43ae42e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 do understand that this is related to
xk6
(somewhat) and as this it seems like a good idea to keep it in the same repo, but:Having gone through the code - none of the new code is dependent on
xk6
or ... even specific about xk6. But it does add more dependency and makes the README harder to read by making it bigger.The tool seems to be useful as well for any other projects trying to align their dependencies between different versions.
As such it seems to me it will be more useful to:
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 agree that from an organizational point of view this tool should be its own module and/or repository.
However, considering that the tool itself is small and, for being a new tool, pretty much unused, I find hard to justify at this point of the tool's lifespan the operational burden of creating a new repository with is corresponding permissions, settings, ci/cd and release pipelines, etc.
I think we can always split the repo (or merge the command) if it starts making sense later on, while benefiting from the simpler operation and discoverability that sharing the repo provides.
Regarding:
I agree and mentioned this here. I'll be okay with reverting 8ae8675 and switch back to the homemade string parser that does not have any external dependency at the cost of perhaps being less conformant with the go.mod spec, if you're okay with that tradeoff.
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.
@roobre I miss this during my reviews, sorry. Why not create a dedicated
go.mod
file? This is an independent project in the end, being in the same repo doesn't force us to have the same project from Go's point of view.If we can divide the Go Modules then I share the same feeling as you about having a different repository. A dedicated repo for only one file sounds a bit too much. IMHO.
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.
@codebien No problem, I think creating a different go.mod should be okay. As we already have a
go.mod
in the root though, do you have any preference about where to put the new one? Wouldcmd/xk6-depsync/go.mod
be okay? Should I also vendor the deps of the new module?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.
Yep, it should be the idiomatic way these days for it. If you can do a quick research to confirm even better.
I don't think it's required. It's a very simple tool. But I don't have a strong opinion here.
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.
Gave this a try. In order for go build to work, I had to add a go workspace. Otherwise the toolchain does not seem to understand it is a different module:
I've tested using my fork that
go install
works as expected: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.
@codebien Eager to hear your thoughts on this :)
If you're not positive about the approach (go.work file and all) then perhaps we should go for the route that @mstoykov suggested and just create a new repo for this tool.
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.
Moving the project to another repo also has the benefit of making the tool more general - taking the repo it is supposed to take in as an argument.
Also both tools won't get updated eveyr time the other is
At this point I have to say that :
I would expect there is even a template repo that makes the above a single click
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.
@roobre I'm fine with moving it. 👍
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.
Can you give an example of this?
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.
Yes, we faced this in the
xk6-disruptor
project. We introduced this code as a script in this commit, which at the time yielded the following command:Running said command with Go 1.21 produced the following changes, which I would pretty much expect:
With Go 1.19, however, yielded these changes instead:
Which were pretty unexpected to me, and were pretty much contradicting the command, which asked for
google.golang.org/[email protected]
and yet yielded: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.
Consider using https://pkg.go.dev/golang.org/x/[email protected]/modfile#Parse instead of custom parsing logic, it would be a more robust solution....
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've given this a swing in the latest two commits. I wouldn't say I'm against it, but I'm not very convinced either. I don't see the code significantly less complex or significantly shorter either.
LMK what you think about this. If you prefer it this way, I'm happy letting it stay, otherwise I can drop the commits and get back to the original implementation. Your call :)
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 didn't recommend it because of the complexity or length of the code. If there is a well-maintained, high-quality library for reading a file format, it is advisable to use it. This way you can avoid that your own parser does not exactly follow the file format specification.
BTW this is just a suggestion, it's up to the code maintainer to decide :)
( @olegbespalov ? )
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.