-
Notifications
You must be signed in to change notification settings - Fork 1.4k
CORS-4266: Add generated DeepCopy implementations for pkg/types/installconfig #10025
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
base: main
Are you sure you want to change the base?
Conversation
aafbc03 to
e3465e9
Compare
|
[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 |
| ) | ||
|
|
||
| //go:generate go run ../../vendor/sigs.k8s.io/controller-tools/cmd/controller-gen crd:crdVersions=v1 paths=. output:dir=../../data/data/ | ||
| //go:generate go run ../../vendor/k8s.io/code-generator/cmd/deepcopy-gen --output-file zz_generated.deepcopy.go ./... |
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.
Here is where the generator is actually invoked. This ties in with existing codegen, and hack/verify_codegen.sh
e68c362 to
76157f1
Compare
76157f1 to
f99bfe3
Compare
|
/retitle CORS-4266: Add generated DeepCopy implementations for pkg/types/installconfig |
|
@dlom: This pull request references CORS-4266 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@dlom: This pull request references CORS-4266 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
1 similar comment
|
@dlom: This pull request references CORS-4266 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
|
@dlom: This pull request references CORS-4266 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
Iff the intent of adding kubebuilder/DeepCopy code generation is to enable these types to be used in CRD definitions, it stands to reason that these CRDs should be usable in a k8s cluster. UniqueItem=true is not permitted on CRDs for k8s. This might be controversial because it relaxes validation requirements
d4fcf54 to
91cb5e1
Compare
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
@dlom: This pull request references CORS-4266 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@dlom: The following tests failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
patrickdillon
left a comment
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 looks good. Left some minor comments, which I think should be addressed.
Regarding the last commit, removing uniqueitems I think that's fine. I reviewed the validation code and we have validation in place that ensures if 2 IPs are provided, one must be ipv4 and one ipv6, so therefore they must be unique. i.e. the validation is redundant and already in place.
| IP: net.IP{10, 0, 0, 0}, | ||
| Mask: net.IPv4Mask(255, 0, 0, 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.
I don't understand this.. Is this seeding with an incorrect value, and it should be overwritten with the correct value?
Seems like it should just be empty (&IPNet{})
| ipNetIn.IPNet = net.IPNet{ | ||
| IP: net.IP{192, 168, 10, 10}, | ||
| Mask: net.IPv4Mask(255, 255, 255, 255), | ||
| } | ||
| if ipNetOut.String() == ipNetIn.String() { | ||
| t.Fatalf("%v (%q) == %v (%q)", ipNetOut, ipNetOut.String(), ipNetIn, ipNetIn.String()) | ||
| } |
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 assertion also seems a little weird. Can you double check this?
| @@ -1,5 +1,6 @@ | |||
| // Package azure contains Azure-specific structures for installer | |||
| // configuration and management. | |||
| // +k8s:deepcopy-gen=package | |||
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.
does the annotation only need to be on a single file in the package? Typically platform.go is the "main" file for each platform, and prima face seems to mea better candidate than doc.go, though I'm open to doc.go if there's a rationale
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 generation is governed by comment tags in the source. Any package may request DeepCopy generation by including a comment in the file-comments of a doc.go file, of the form:
shows what I know!
Closes #256 (for real this time)
xref: CORS-4266
See also: openshift/hive#2731
Additionally, this PR removes the
UniqueItemsvalidator on the IP address tuples, as explained in the commit message