8000 Actually, vendor everything by sixolet · Pull Request #59 · knative/client · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Actually, vendor everything #59

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

Merged
merged 3 commits into from
Apr 5, 2019
Merged

Conversation

sixolet
Copy link
Contributor
@sixolet sixolet commented Apr 4, 2019

We were getting errors when trying to build in CI when a backing repository 502'd, so let's vendor all our deps.

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sixolet

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 4, 2019
@bbrowning
Copy link
Contributor

With this change, it means go modules are no longer technically required to build the client repo. And, we should probably add hack/update-deps.sh and hack/verify-codegen.sh to both update the deps and let CI verify vendor doesn't drift from go.mod/go.sum. And we can likely remove our license checking hack from test/presubmit-tests.sh. So, a couple of scripts and an update to DEVELOPMENT.md indicating how to use them.

Adding a lgtm with a hold. I'm ok to merge this now and address the related changes in subsequent PRs, but punting that decision to you with the hold.

@bbrowning
Copy link
Contributor

Helps if I actually do the prow commands as well...

/lgtm
/hold

@knative-prow-robot knative-prow-robot added lgtm Indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 4, 2019
@sixolet
Copy link
Contributor Author
sixolet commented Apr 5, 2019

How much would this help you get e2e tests up and running if we landed it?

@bbrowning
Copy link
Contributor
8000 bbrowning commented Apr 5, 2019

This PR should solve the e2e flake issues as-is. After it lands I'll kick off tests on the e2e PR a few times to confirm.

@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 5, 2019
@sixolet
Copy link
Contributor Author
sixolet commented Apr 5, 2019

According to golang/go#27348 there is no good way to actually verify the contents of the vendor directory. Do you happen to know if I'm missing something?

I added a trivial script to go mod vendor things, and I think that might be the best compromise for now. What do you think?

@bbrowning
Copy link
Contributor

Thi seems like a good compromise. I'll poke at how we actually verify vendor in other Knative repos - I think they do some special trick with copying files to a tmp directory, vendoring, and comparing. But that can wait.

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 5, 2019
@bbrowning
Copy link
Contributor

/hold cancel

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 5, 2019
@knative-prow-robot knative-prow-robot merged commit fb58718 into knative:master Apr 5, 2019
@adrcunha
Copy link
Contributor
adrcunha commented Apr 5, 2019

I'll poke at how we actually verify vendor in other Knative repos - I think they do some special trick with copying files to a tmp directory, vendoring, and comparing.

That's correct:

https://github.com/knative/serving/blob/master/hack/verify-codegen.sh

navidshaikh added a commit to navidshaikh/client that referenced this pull request Aug 13, 2019
 - Earlier we were only running few tests
 - This runs the e2e tests present in upstream repository
coryrc pushed a commit to coryrc/client that referenced this pull request May 14, 2020
$0 isn't available when sourcing a script.

Plus, update `README.md` to clarify how these files appear in other repos.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0