catch go mod download overwriting changes to vendor #3008
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jaxesn
changed the title
jaxesn
force-pushed
the
jgw/catch-overwrite-vendor
branch
from
3a8de5d to
a6a15bc
Compare
jaxesn
added a commit
to aws/eks-anywhere-build-tooling
that referenced
this pull request
Jun 3, 2024
harbor vendors its go mods and there used to be a patch that changed files in the vendor directory. setting this flag was necessary to avoid losing the change since go mod download happened after patching. see aws/eks-distro#3008 for more info. I will send another PR adding a similar check to this repo to catch this kind of in the future automatically.
eks-distro-bot
pushed a commit
to aws/eks-anywhere-build-tooling
that referenced
this pull request
Jun 3, 2024
* harbor no longer needs to skip go mod download harbor vendors its go mods and there used to be a patch that changed files in the vendor directory. setting this flag was necessary to avoid losing the change since go mod download happened after patching. see aws/eks-distro#3008 for more info. I will send another PR adding a similar check to this repo to catch this kind of in the future automatically. * Update CHECKSUMS
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
rcrozean
force-pushed
the
jgw/catch-overwrite-vendor
branch
from
0e277c4 to
a11dc8c
Compare
|
/retest |
|
/retest aws-cloud-controller-manager-1-26-presubmit |
|
@rcrozean: The
Use
In response to this:
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. |
|
/test aws-cloud-controller-manager-1-26-presubmit |
rcrozean
force-pushed
the
jgw/catch-overwrite-vendor
branch
from
a11dc8c to
2bbebd2
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Issue #, if available:
Description of changes:
From pretty much the beginning of eks-d and then eks-a after it, we have run
go mod downloadbefore building. Some upstream projects, such as k/k, actually check in their vendor folder so in those cases we do not technically need to rungo mod vendor(and in the case of k/k we actually do not, never have) but we do anyway to be consistent and avoid having to have made a case by case decision. This was also important for reproducible builds, there is some oddities with go dependencies that have been download vs checked in and pulled. I have previous PRs which talk about this. Always runninggo mod vendorensured all projects would build regardless of if the vendor dir was checked in or not.About a year ago, the
harborbuild in eks-a was updated with a patch that actually did patch something in the vendor directory. At that time we addedGO_MODS_VENDOREDin aws/eks-anywhere-build-tooling#2078 to allow projects to skip runninggo mod vendor.Recently a patch was added to CCM for versions 1.28+ which 1) adds the vendor directory and 2) patches something in that directory. As is, this patch is not actually taking affect because it gets overwritten when the
go mod vendorcall is ran. The fix, which @markapruett is taking, is to port over the skipping mechanism from eks-a to eks-d and setting GO_MODS_VENDORED to true for the ccm build.While discussing with Mark this nuance, I was thinking of ways we could catch this and avoid the silent "failure". This is my attempt, if the vendor dir exists before running
go mod vendorthen use git to determine if anything changed in that folder due togo mod vendor, if so, then error.With this change it will require that any time we patch a go.mod/sum file for a project with vendored deps, we need to make sure the patch includes the vendor folder updates, otherwise git will see a difference and trigger this error. The fix should be pretty easy/obvious in this case.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.