-
Notifications
You must be signed in to change notification settings - Fork 150
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
MCO-483: generate machineconfiguration (MCO) clients as part of MCO API migration #243
Conversation
@jkyros: This pull request references MCO-483 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 kubernetes/test-infra repository. |
Skipping CI for Draft Pull Request. |
/test all |
dc41f6a
to
e5f854e
Compare
/test all |
/test all |
/test all |
echo "Generating applyconfigurations specifically for MCO" | ||
applyconfigurationgen_external_apis_csv="$(codegen::join , "${FQ_APIS[@]}")" | ||
applyconfigurations_package="${OUTPUT_PKG}/${CLIENTSET_PKG_NAME:-applyconfigurations}" | ||
${APPLYCONFIGURATION_GEN} \ | ||
--output-package "${applyconfigurations_package}" \ | ||
--input-dirs "${applyconfigurationgen_external_apis_csv}" \ | ||
--external-applyconfigurations k8s.io/api/core/v1.ObjectReference:k8s.io/client-go/applyconfigurations/core/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.
So the MCO embeds a corev1.ObjectReference
here, which makes applyconfiguration-gen generate code that doesn't compile.
# github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1
vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfigpoolstatusconfiguration.go:27:11: cannot use &value (value of type *string) as type string in assignment
vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfigpoolstatusconfiguration.go:35:16: cannot use &value (value of type *string) as type string in assignment
vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfigpoolstatusconfiguration.go:43:11: cannot use &value (value of type *string) as type string in assignment
vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfigpoolstatusconfiguration.go:51:10: cannot use &value (value of type *types.UID) as type types.UID in assignment
vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfigpoolstatusconfiguration.go:59:17: cannot use &value (value of type *string) as type string in assignment
vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfigpoolstatusconfiguration.go:67:22: cannot use &value (value of type *string) as type string in assignment
vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1/machineconfigpoolstatusconfiguration.go:75:16: cannot use &value (value of type *string) as type string in assignment
make: *** [Makefile:40: _build-machine-config-daemon] Error 2
It looks like applyconfiguration-gen
includes the following as external by default (which I believe is why this is an issue with corev1.ObjectReference
but not an issue with the metav1.TypeMeta/metav1.ObjectMeta
embedded fields that seem to work:
k8s.io/apimachinery/pkg/apis/meta/v1.TypeMeta:k8s.io/client-go/applyconfigurations/meta/v1,
k8s.io/apimachinery/pkg/apis/meta/v1.ObjectMeta:k8s.io/client-go/applyconfigurations/meta/v1,
k8s.io/apimachinery/pkg/apis/meta/v1.OwnerReference:k8s.io/client-go/applyconfigurations/meta/v1
If I add corev1.ObjectReference
as external, and point it to its appyconfiguration:
k8s.io/api/core/v1.ObjectReference:k8s.io/client-go/applyconfigurations/core/v1
Everything generates right for the MCO, but it changes a bunch of the other packages that were originally generated without the presence of v1.ObjectReferenceApplyConfiguration
, and that seems...not good...because they've already been working well for quite some time.
So...I only added it when we're generating for the MCO and left it out for everyone else?
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 the "additional damage" in other packages if I let them generate with corev1 as external: 3367954
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.
It's curious that the MCO blows up but others haven't, is there any difference in how the object reference is being used in the other APIs? Pointer vs reference per chance?
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.
There are examples in the API of both pointer/non-pointer usages, but ours is the only one that is embedded:
apps/v1/types.go: To corev1.ObjectReference `json:"to" protobuf:"bytes,2,opt,name=to"`
apps/v1/types.go: From corev1.ObjectReference `json:"from" protobuf:"bytes,3,opt,name=from"`
apps/v1/types.go: From corev1.ObjectReference `json:"from" protobuf:"bytes,1,opt,name=from"`
apps/v1/types.go: From corev1.ObjectReference `json:"from" protobuf:"bytes,1,opt,name=from"`
authorization/v1/types.go: Subjects []corev1.ObjectReference `json:"subjects" protobuf:"bytes,4,rep,name=subjects"`
authorization/v1/types.go: RoleRef corev1.ObjectReference `json:"roleRef" protobuf:"bytes,5,opt,name=roleRef"`
authorization/v1/types.go: Subjects []corev1.ObjectReference `json:"subjects" protobuf:"bytes,4,rep,name=subjects"`
authorization/v1/types.go: RoleRef corev1.ObjectReference `json:"roleRef" protobuf:"bytes,5,opt,name=roleRef"`
build/v1/types.go: FromRef *corev1.ObjectReference `json:"fromRef,omitempty" protobuf:"bytes,2,opt,name=fromRef"`
build/v1/types.go: Config *corev1.ObjectReference `json:"config,omitempty" protobuf:"bytes,9,opt,name=config"`
build/v1/types.go: From corev1.ObjectReference `json:"from" protobuf:"bytes,1,opt,name=from"`
build/v1/types.go: From corev1.ObjectReference `json:"from" protobuf:"bytes,1,opt,name=from"`
build/v1/types.go: From *corev1.ObjectReference `json:"from,omitempty" protobuf:"bytes,1,opt,name=from"`
build/v1/types.go: From corev1.ObjectReference `json:"from" protobuf:"bytes,1,opt,name=from"`
build/v1/types.go: To *corev1.ObjectReference `json:"to,omitempty" protobuf:"bytes,1,opt,name=to"`
build/v1/types.go: From *corev1.ObjectReference `json:"from,omitempty" protobuf:"bytes,2,opt,name=from"`
build/v1/types.go: TriggeredByImage *corev1.ObjectReference `json:"triggeredByImage,omitempty" protobuf:"bytes,3,opt,name=triggeredByImage"`
build/v1/types.go: From *corev1.ObjectReference `json:"from,omitempty" protobuf:"bytes,4,opt,name=from"`
image/v1/types.go: From *corev1.ObjectReference `json:"from,omitempty" protobuf:"bytes,3,opt,name=from"`
image/v1/types.go: From corev1.ObjectReference `json:"from" protobuf:"bytes,1,opt,name=from"`
image/v1/types.go: From corev1.ObjectReference `json:"from" protobuf:"bytes,1,opt,name=from"`
machine/v1beta1/types_machinehealthcheck.go: RemediationTemplate *corev1.ObjectReference `json:"remediationTemplate,omitempty"`
machine/v1beta1/types_machine.go: NodeRef *corev1.ObjectReference `json:"nodeRef,omitempty"`
security/v1/types.go: AllowedBy *corev1.ObjectReference `json:"allowedBy,omitempty" protobuf:"bytes,1,opt,name=allowedBy"`
template/v1/types.go: Ref corev1.ObjectReference `json:"ref,omitempty" protobuf:"bytes,1,opt,name=ref"`
template/v1/types.go: TemplateInstance corev1.ObjectReference `json:"templateInstance" protobuf:"bytes,1,opt,name=templateInstance"`
template/v1/types.go: Secret corev1.ObjectReference `json:"secret" protobuf:"bytes,2,opt,name=secret"`
user/v1/types.go: User corev1.ObjectReference `json:"user" protobuf:"bytes,4,opt,name=user"`
user/v1/types.go: Identity corev1.ObjectReference `json:"identity,omitempty" protobuf:"bytes,2,opt,name=identity"`
user/v1/types.go: User corev1.ObjectReference `json:"user,omitempty" protobuf:"bytes,3,opt,name=user"`
vs
machineconfiguration/v1/types.go: PullSecret *corev1.ObjectReference `json:"pullSecret,omitempty" protobuf:"bytes,9,opt,name=pullSecret"`
---> machineconfiguration/v1/types.go: corev1.ObjectReference `json:",inline" protobuf:"bytes,1,rep,name=objectReference"`
machineconfiguration/v1/types.go: Source []corev1.ObjectReference `json:"source,omitempty" protobuf:"bytes,2,rep,name=source"`
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.
Ahh, right, in general for APIs we try to avoid embedding structs like that, but, it's done now.
In theory you could change the go type without causing serialization errors, copying the fields from the object reference into the parent struct, that would mean you don't have this hack any more.
Also, we should remove the protobuf tags, they aren't used for CRDs
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.
In theory you could change the go type without causing serialization errors, copying the fields from the object reference into the parent struct, that would mean you don't have this hack any more.
Something like this? https://gist.github.com/jkyros/db4d27fb28b38fa2ea170164ebfabae9#file-hypothetical_atrocity-go That feels like it should be a crime 😛 I think it would work, but there is stuff in the MCO that cares that it's an ObjectReference (mostly tests) and we'd need to defuse all of that.
Would you entertain letting us keep this PR's MCO-only applyconfig hack for now (or something similar -- I don't want to make your update script ugly) until we can do some rearrangement? The applyconfigurations generated by this hack do seem to be proper and work, e.g. https://gist.github.com/jkyros/db4d27fb28b38fa2ea170164ebfabae9#file-main-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.
Yeah the gist looks like what I meant. Since the serialization is the same, it's fine for a CRD to make a change like that to the go type, I got second opinions on that and a few of us are in agreement 😅
I'm happy to let you carry on like this, but, can we stick a pin in this to fix it before the end of 4.15? I'd like a TODO to remove this hack
bf3c410
to
d154d1e
Compare
d154d1e
to
303616e
Compare
@jkyros: This pull request references MCO-483 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 kubernetes/test-infra repository. |
@jkyros: This pull request references MCO-483 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 kubernetes/test-infra repository. |
@jkyros: This pull request references MCO-483 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 kubernetes/test-infra repository. |
@jkyros: This pull request references MCO-483 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 kubernetes/test-infra repository. |
/test all |
303616e
to
d1f02b9
Compare
d1f02b9
to
91ea131
Compare
The MCO's API unfortunately has embedded a corev1.ObjectReference for years. As part of the MCO's API migration, we tried to generate clients, but this resulted in broken applyconfigurations for the MCO, because applyconfiguration-gen doesn't regard the corev1.ObjectReference as an external type without being explicitly specified, so when it calculates "reachability" of the type, it's not in the list. This commit adds machineconfiguration to the list of packages for generation, and also temporarily splits the MCO's applyconfiguration-gen commands out into a special MCO-only block so it can reference corev1 as external and generate properly without affecting generation for the other packages here -- at least until the MCO can get rid of that embedded reference.
4d88844
to
d08116b
Compare
d08116b
to
c7f9314
Compare
@jkyros: all tests passed! 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. |
Alright, this should be ready now too. I've taken out the replace directive that pointed at my branch, this is successfully generating against the API that was merged in openshift/api#1453. |
# TODO(jkyros): this is a temporary hack to ensure proper generation until the MCO can sort | ||
# out their embedded corev1.ObjectReference in MachineConfigPoolStatusConfiguration, which needs | ||
# to be resolved by the end of release-4.15 so this hack can be removed |
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.
Is this being tracked?
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.
Yes, I created MCO-726 for this purpose. Since we're in 4.15 now I moved it into our API migration epic, so it will be required to be completed in order for MCO to close the epic 😄
/lgtm |
@jkyros: This pull request references MCO-483 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 story to target the "4.15.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 kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jkyros, JoelSpeed 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 |
@jkyros: This pull request references MCO-483 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 story to target the "4.15.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 kubernetes/test-infra repository. |
MCO-483: generate machineconfiguration (MCO) clients as part of MCO API migration Signed-off-by: Charlie Doern <[email protected]>
MCO-483: generate machineconfiguration (MCO) clients as part of MCO API migration Signed-off-by: Charlie Doern <[email protected]>
MCO-483: generate machineconfiguration (MCO) clients as part of MCO API migration Signed-off-by: Charlie Doern <[email protected]>
This generates the clients matching openshift/api#1453. Once that one merges, I will come back and update the vendoring and try to get this one in.
This does include a hack/workaround for the MCO having an embedded ObjectReference (see: #243 (review)) which the MCO intends to address during 4.15. I've created MCO-726 to track the API cleanup and removal of this hack.
Closes: MCO-483