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

cmd/operator-sdk/internal/genutil: relax version dir check in parseGroupVersions() #1525

Merged
merged 4 commits into from
Jun 10, 2019

Conversation

estroz
Copy link
Member

@estroz estroz commented Jun 5, 2019

Description of the change: remove requirement that the API dir structure pkg/apis/<group>/<version> have <version> match something like v1alpha1, and instead check if <version> is a dir (or dir hierarchy) that contains files.

Motivation for the change: having a API dir shared by multiple versions, ex pkg/apis/operators/shared, is valid and should be considered when running a code generator.

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 5, 2019
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@joelanford
Copy link
Member

Before merging, we should double check with @awgreene that this solves his problem.

@awgreene
Copy link
Member

awgreene commented Jun 6, 2019

Before merging, we should double check with @awgreene that this solves his problem.

Thanks for working on this @joelanford and @estroz!

I pulled the relax-fqapis-check branch and ran the sdk after updating the phase types to include the deepcopy gen code:

// Phase describes the phase the object is in
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
type Phase struct {
	// Name of the phase
	Name string `json:"name,omitempty"`

	// A human readable message indicating why the object is in this phase
	Message string `json:"message,omitempty"`
}

// ObjectPhase describes the phase of a Marketplace object is in along with the
// last time a phase transition occurred and when the object was last updated
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
type ObjectPhase struct {
	// Current phase of the object
	Phase `json:"phase,omitempty"`

	// Last time the object has transitioned from one phase to another
	LastTransitionTime metav1.Time `json:"lastTransitionTime,omitempty"`

	// Last time the status of the object was updated
	LastUpdateTime metav1.Time `json:"lastUpdateTime,omitempty"`
}

I then ran the following command:

$ operator-sdk generate k8s
INFO[0000] Running deepcopy code-generation for Custom Resource group versions: [operators:[shared v1 v2], ] 
INFO[0001] Code-generation complete.                           

It looks like the shared dir was inspected, but there was no code generated. Any thoughts?

@estroz
Copy link
Member Author

estroz commented Jun 6, 2019

@awgreene do you have doc.go and register.go in pkg/apis/operator/shared? AFAIK only doc.go is necessary.

Also you might have to embed the following in Phase:

type Phase struct {
	metav1.TypeMeta   `json:",inline"`
	metav1.ObjectMeta `json:"metadata,omitempty"`

	...
}

While testing with your code I got compilation errors because Phase didn't implement the k8s.io/apimachinery/pkg/runtime.Object method, which requires a GetObjectKind() method, ex. from metav1.TypeMeta.

@awgreene
Copy link
Member

awgreene commented Jun 6, 2019

type Phase struct {
	metav1.TypeMeta   `json:",inline"`
	metav1.ObjectMeta `json:"metadata,omitempty"`

Thanks for looking into this @estroz, is there a reason why this needs to be added if it's moved into it's own package? When phase was in the v1 package it did not need the field.

@estroz
Copy link
Member Author

estroz commented Jun 6, 2019

@awgreene can you link the rest of your code?

@awgreene
Copy link
Member

awgreene commented Jun 6, 2019

@awgreene can you link the rest of your code?

awgreene/operator-marketplace@7d6b64f

I copied the old deepcopy code into this commit earlier but the only change other change involved moving the phase_types.go file.

@estroz
Copy link
Member Author

estroz commented Jun 6, 2019

Ah I see the issue. The above code has // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object above ObjectPhase and Phase which forces DeepCopyObject() to be generated, but that is not necessary. Remove those lines and the deepcopy code should build fine.

@awgreene
Copy link
Member

awgreene commented Jun 6, 2019

Ah I see the issue. My test code had // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object above ObjectPhase and Phase which forces DeepCopyObject() to be generated, but that is not necessary. Disregard my field addition comment.

Great - adding the doc file seems to have fixed everything! Thanks @estroz

Copy link
Contributor

@hasbro17 hasbro17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@estroz estroz merged commit fd7d925 into operator-framework:master Jun 10, 2019
@estroz estroz deleted the relax-fqapis-check branch June 10, 2019 21:19
lilic pushed a commit to lilic/operator-sdk that referenced this pull request Jun 11, 2019
…roupVersions()` (operator-framework#1525)

* cmd/operator-sdk/internal/genutil: relax version dir check in parseGroupVersions()

* CHANGELOG.md: add change for relaxing API version format check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants