Skip to content
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 update-codegen.sh when run outside GOPATH #18

Closed
wants to merge 1 commit into from

Conversation

Cynocracy
Copy link
Contributor

@Cynocracy Cynocracy commented Jun 19, 2020

Fix update-codegen.sh after modules migration by hacking generate-groups and generate-internal-groups.

Addresses knative/pkg#1287 if accepted will likely attempt an upstream into library.sh.

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 19, 2020
@knative-prow-robot knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 19, 2020
@Cynocracy Cynocracy changed the title Fix update-codegen.sh [WIP] Fix update-codegen.sh Jun 19, 2020
@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 19, 2020
@knative-prow-robot knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 19, 2020
@Cynocracy Cynocracy changed the title [WIP] Fix update-codegen.sh [WIP] Fix update-codegen.sh when run outside GOPATH Jun 19, 2020
@Cynocracy Cynocracy force-pushed the fixupdatecodegen branch 2 times, most recently from c78a907 to 5f13329 Compare June 19, 2020 06:24
@knative-prow-robot knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 19, 2020
@knative-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Cynocracy
To complete the pull request process, please assign grantr
You can assign the PR to them by writing /assign @grantr in a comment when ready.

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

@Cynocracy Cynocracy changed the title [WIP] Fix update-codegen.sh when run outside GOPATH Fix update-codegen.sh when run outside GOPATH Jun 19, 2020
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 19, 2020
@Cynocracy
Copy link
Contributor Author

/assign @grantr

@Cynocracy
Copy link
Contributor Author

As an answer to the question: why not simply update client-go?

The latest version uses the OpenAPIv2 package instead of openapiv2, and updating client-go leads to a diamond dependency problem that can't be solved with replace directives because you can't alias two packages to the same module (OpenAPIv2, OpenAPIv3) -> (openapiv2, openapiv3)

@Cynocracy
Copy link
Contributor Author

/assign @n3wscott as I hear you've played around in the area 😄

@n3wscott
Copy link
Contributor

Can you confirm that the client gen produces code in the go mod file path? I was testing and was only able to change the package, but that package was always rooted in $GOPATH

@Cynocracy
Copy link
Contributor Author

Can confirm, This code was generated using just ./hack/update-codegen.sh outside of GOPATH

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 22, 2020
@n3wscott
Copy link
Contributor

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 22, 2020
@knative-prow-robot
Copy link

New changes are detected. LGTM label has been removed.

@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 23, 2020
@knative-prow-robot
Copy link

@Cynocracy: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-knative-networking-unit-tests 47ce700 link /test pull-knative-networking-unit-tests
pull-knative-networking-build-tests 47ce700 link /test pull-knative-networking-build-tests

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2020
@knative-prow-robot
Copy link

@Cynocracy: PR needs rebase.

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.

@tcnghia
Copy link
Contributor

tcnghia commented Aug 1, 2020

@Cynocracy can you please rebase? thanks

@Cynocracy
Copy link
Contributor Author

Cynocracy commented Aug 2, 2020 via email

@Cynocracy
Copy link
Contributor Author

Cynocracy commented Aug 2, 2020 via email

@Cynocracy Cynocracy closed this Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates the PR's author has signed the CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants