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.

Sign up for GitHub

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

Jump to bottom

Bug 1735728: Add description to operator user facing CRDs. #65

Merged
merged 2 commits into from Aug 26, 2019
Merged

Bug 1735728: Add description to operator user facing CRDs. #65

merged 2 commits into from Aug 26, 2019

Conversation

pliurh
Copy link
Contributor

@pliurh pliurh commented Aug 16, 2019

No description provided.

@openshift-ci-robot
Copy link
Contributor

@pliurh: This pull request references a valid Bugzilla bug. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1735728: Add description to operator user facing CRDs.

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.

@openshift-ci-robot openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 16, 2019
zshi-redhat
zshi-redhat reviewed Aug 23, 2019
View reviewed changes
pkg/apis/sriovnetwork/v1/sriovnetworknodestate_types.go
@@ -53,7 +53,7 @@ type SriovNetworkNodeStateStatus struct {

// +genclient
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

// +kubebuilder:subresource:status
Copy link
Contributor

Choose a reason for hiding this comment

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

curious, does updating nodestate work without this tag before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a bug in operator-sdk, which makes errors the generated CRD. Therefore, we have to do manual change to the generated CRD.

pkg/apis/k8s/v1/zz_generated.deepcopy.go
@@ -1,6 +1,6 @@
// +build !ignore_autogenerated

// Code generated by deepcopy-gen. DO NOT EDIT.
// Code generated by operator-sdk. DO NOT EDIT.
Copy link
Contributor

Choose a reason for hiding this comment

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

did you use different tool this time to generate code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. It caused by the operator-sdk update.

deploy/crds/sriovnetwork_v1_sriovnetworknodepolicy_crd.yaml
@@ -10,8 +10,6 @@ spec:
plural: sriovnetworknodepolicies
singular: sriovnetworknodepolicy
scope: Namespaced
subresources:
Copy link
Contributor

Choose a reason for hiding this comment

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

so nodepolicy status is never used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct

deploy/crds/sriovnetwork_v1_sriovnetwork_crd.yaml
maximum: 4096
minimum: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I noticed the order of max/min vlan numbers have been changed many times, is there particular reason for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CRD file is auto generated by operator-sdk. I don't think the order matters.

deploy/crds/sriovnetwork_v1_sriovnetwork_crd.yaml
@@ -26,22 +26,30 @@ spec:
metadata:
type: object
spec:
description: Specification describing how a NetworkAttachmentDefinition
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we also change the file under manifests/4.2/manifests/4.2/sriov-network-operator-sriovnetwork.crd.yaml?
before this PR, they have same content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Will do.

@zshi-redhat
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2019
@openshift-merge-robot openshift-merge-robot merged commit d8c9074 into openshift:master Aug 26, 2019
@openshift-ci-robot
Copy link
Contributor

@pliurh: All pull requests linked via external trackers have merged. Bugzilla bug 1735728 has been moved to the MODIFIED state.

In response to this:

Bug 1735728: Add description to operator user facing CRDs.

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.

pliurh pushed a commit to pliurh/sriov-network-operator-1 that referenced this pull request Apr 1, 2021
@moshe010
Merge pull request openshift#65 from manuelbuil/master
f919298 
Allow modifying RESOURCE_PREFIX
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zshi-redhat zshi-redhat zshi-redhat left review comments

@fepan fepan Awaiting requested review from fepan

@s1061123 s1061123 Awaiting requested review from s1061123

Assignees

@zshi-redhat zshi-redhat

Labels
bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@pliurh @openshift-ci-robot @zshi-redhat @openshift-merge-robot