-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix: add v2 to module path #8169
Conversation
Codecov Report
@@ Coverage Diff @@
## main #8169 +/- ##
==========================================
- Coverage 70.48% 66.11% -4.37%
==========================================
Files 515 601 +86
Lines 23150 29483 +6333
==========================================
+ Hits 16317 19494 +3177
- Misses 5776 8528 +2752
- Partials 1057 1461 +404
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Codecov seems to be comparing to an old commit (290280e was merged over a year ago). Other PRs show the same coverage decrease, so I think this is not related to my module path change. @aaron-prindle, is there anything else I need to do for this PR? |
@sethrylan thanks for flagging and submitting this. I think the PR is good as is w/ no changes, I am holding off merging until after our next minor release - EDIT: Sorry I just saw:
I plan on merging this right after Dec 7th, will that work for your team? |
Yes, that works great. Thank you for the quick review, and let me know if there's anything else for this PR. |
@aaron-prindle, I've merged the latest commits from main to this branch, and updated the |
Sorry for the delay, our next release was pushed until tomorrow - 12/14/2022. Will get to this by end of this week, sorry for any inconvenience |
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.
Our release is still waiting on some high priority fixes, I've been manually testing this locally and reviewed all of the files touched and things look good. It should be safe to merge now from my testing. Merging now
Thanks for the PR @sethrylan! Out of curiosity how is your team using the skaffold go module currently? Any insights there might help us to better support the use case, thanks! |
Thanks so much @aaron-prindle! There are two ways we use the module:
|
Making triage party happy |
Fixes: #8161
Description
This PR updates the module path to include
v2
.Go modules treat v0 and v1 with relaxed versioning requirements. Incrementing to v2 asks for a new module path:
In skaffold's case, we need
module github.com/GoogleContainerTools/skaffold/v2 v2.0.2
so thatgo get github.com/GoogleContainerTools/skaffold/[email protected]
will work.Go intends for the module path to prevent unintentionally upgrading to a new major version. By adding the version to the module path, developers must deliberately import breaking changes.
I realize that few projects have this issue, because few import the skaffold packages. We do have such a project, and we're looking forward to importing the v2 packages.