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 CRD / Codegen #865
fix CRD / Codegen #865
Conversation
squeed
commented
Nov 4, 2020
- make sure codegen is vendored in
- bump controller-tools
- add in the manually generated annotations
Hi @squeed it does seem like the CI is complaining about the magnet, mind taking a look before I cherry-pick this? Thanks! |
@@ -20,7 +20,7 @@ if ! ( command -v controller-gen > /dev/null ); then | |||
fi | |||
fi | |||
|
|||
"${SCRIPT_ROOT}/vendor/k8s.io/code-generator/generate-groups.sh" deepcopy \ | |||
bash "${SCRIPT_ROOT}/vendor/k8s.io/code-generator/generate-groups.sh" deepcopy \ |
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.
why?
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.
because the shell script isn't executable.
@@ -23,8 +23,8 @@ trap 'rc=$?' ERR | |||
# Go to the root of the repo | |||
cd "$(git rev-parse --show-cdup)" | |||
|
|||
GOFILES=$(find . -path ./vendor -prune -o -name '*.go' | grep -v vendor | grep -v pkg/operator/assets) | |||
GOPKGS=$(go list ./... | grep -v '/vendor/' | grep -v '/generated/' | grep -v pkg/operator/assets) | |||
GOFILES=$(find . -path ./vendor -prune -o -name '*.go' | grep -v vendor) |
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.
fwiw
GOFILES=$(find . -path ./vendor -prune -o -name '*.go' -print)
works too. (ie, explicitly telling it to print filenames that match the -name
predicate overrides the default behavior of printing filenames that match any predicate)
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.
all of this is silly - filed SDN-1342 to switch to golangci-lint.
# because of controller-tools bug 302 | ||
echo "controller-gen not found, installing sigs.k8s.io/controller-tools@83f6193..." | ||
if ( ! ( command -v controller-gen > /dev/null ) || test "$(controller-gen --version)" != "Version: v0.4.0" ); then | ||
echo "controller-gen not found or out-of-date, installing sigs.k8s.io/controller-tools@v0.4.0..." |
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.
if [[ $(controller-gen --version 2>/dev/null) != "Version: v0.4.0" ]]; then
?
also, "incorrect version", not "out of date", though do we really want to be blocking people from using v0.4.1+? Are we assuming API incompatibilities?
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.
I hate it when I do codegen (in any of its forms) and the output changes in ways I don't understand. So, I prefer to pin versions.
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.
What would be changed without this pinning exactly?
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.
@danielmellado if we don't pin a specific version, then people who run hack/update-codegen.sh
might get different output depending on the version of the binary installed on their local machine.
--- | ||
apiVersion: apiextensions.k8s.io/v1beta1 | ||
# This file is automatically generated. DO NOT EDIT | ||
apiVersion: apiextensions.k8s.io/v1 |
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.
Not sure who to ask, but do you also want to update the CRD in https://github.com/openshift/cluster-network-operator/blob/master/bindata/network/openshift-sdn/001-crd.yaml#L2 to v1
as well? (from https://bugzilla.redhat.com/show_bug.cgi?id=1881636#c4, this BZ was originally for upgrading all the CRDs in openshift/api to v1)
If so, you may want to do it here and link this PR to that bugzilla
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.
This is just to fix codegen; let's make it a separate PR.
This isn't going to pass until openshift/release#13489 merges. |
/test unit |
@squeed: The specified target(s) for
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. |
hey @tssurya, I know this is blocking you - want to review it? |
/retest |
/hold |
/retest |
This has no effect besides forcing us to vendor k8s code-gen.
This bumps the controller-tools version, and adds a silly hack to set our "release profile" again.
Test failures are spurious. I think we can merge this. Someone want to lgtm? |
/hold cancel |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danielmellado, squeed 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 |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
oops, too late to the party, I need to get better with doing reviews diligently but thanks for doing this, I'll rebase my patch on this. |
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |