-
Notifications
You must be signed in to change notification settings - Fork 161
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
Define scripts/ as a submodule #2141
Define scripts/ as a submodule #2141
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chizhg 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 |
Looks promising, can you fix the build error first? |
bf3d5fa
to
b065380
Compare
b065380
to
f344e7b
Compare
I didn't find an elegant way to fix the build error. It seems I think we can just skip building the submodule it contains mainly scripts, and since we are already skipping a few directories, the approach is not that dirty. |
I couldn't repro the error locally either, maybe we should figure out why this happened instead of going straight to the workaround? |
/hold |
fdaa21a
to
2ba4dd3
Compare
2ba4dd3
to
624ebc0
Compare
e3a0bee
to
d66f654
Compare
I can reproduce the error by running There are two issues with this command:
Given the first issue and the fact the submodule only contains a |
d66f654
to
330e2d0
Compare
/lgtm |
/hold cancel |
What this PR does, why we need it:
I did some experiment with Go multiple modules to vendor
scripts/
, and did not see any issues.The only changes needed in each repo will be changing the floating deps from
knative.dev/test-infra
toknative.dev/test-infra/scripts
in for example https://github.com/knative/serving/blob/master/hack/update-deps.sh#L35. And even if it's not changed, it can still work in the old way.Which issue(s) this PR fixes:
Fixes #2078
/cc @chaodaiG @albertomilan