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

OADP-2959: Add missing descriptions to all CRDs #1200

Merged
merged 1 commit into from Nov 2, 2023

Conversation

mpryc
Copy link
Contributor

@mpryc mpryc commented Oct 26, 2023

Adds missing descriptions to the following CRDs:

 - DataDownload
 - DataUpload
 - VolumeSnapshotBackup
 - VolumeSnapshotRestore
 - CloudStorage

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2023
@openshift-ci
Copy link

openshift-ci bot commented Oct 26, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@mpryc mpryc force-pushed the OADP-2959 branch 2 times, most recently from bcdde5c to 52ee895 Compare October 31, 2023 13:25
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2023
@mpryc mpryc changed the title DNM: Initial description work [OADP-2959] Add missing descriptions to all CRDs Oct 31, 2023
@mpryc mpryc marked this pull request as ready for review October 31, 2023 13:26
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 31, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2023
@mpryc mpryc changed the title [OADP-2959] Add missing descriptions to all CRDs OADP-2959 Add missing descriptions to all CRDs Oct 31, 2023
@mpryc
Copy link
Contributor Author

mpryc commented Oct 31, 2023

/jira refresh

@openshift-ci-robot
Copy link

@mpryc: No Jira issue is referenced in the title of this pull request.
To reference a jira issue, add 'XYZ-NNN:' to the title of this pull request and request another refresh with /jira refresh.

In response to this:

/jira refresh

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.

@mpryc mpryc changed the title OADP-2959 Add missing descriptions to all CRDs OADP-2959: Add missing descriptions to all CRDs Oct 31, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Oct 31, 2023

@mpryc: This pull request references OADP-2959 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 bug to target either version "4.15." or "openshift-4.15.", but it targets "OADP 1.3.0" instead.

In response to this:

Adds missing descriptions to the following CRDs:

- DataDownload
- DataUpload
- VolumeSnapshotBackup
- VolumeSnapshotRestore
- CloudStorage

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 the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 31, 2023
displayName: StartTimestamp
path: startTimestamp
version: v2alpha1
- description: VolumeSnapshotBackup is the Schema for the volumesnapshotbackups
Copy link
Contributor

Choose a reason for hiding this comment

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

VSB and VSR do not exist in master anymore, this can be applied to others branch

@@ -314,6 +314,164 @@ spec:
displayName: Phase
path: phase
version: v1
- description: DataDownload represents a data download of a volume snapshot. There
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets wait for rebase to get @shubham-pampattiwar PR for descriptions of DU and DD CRDs?

Choose a reason for hiding this comment

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

yeah @mateusoliveira43 I can do a cherry-pick against openshift/velero if needed, upstream PR is merged @mpryc

Copy link
Contributor

Choose a reason for hiding this comment

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

We won't be doing any more rebases to konveyor-dev until we've created oadp-1.3 branches, since we're already on the upstream release we're shipping for 1.3 -- any upstream commits needed for 1.3 will need to be cherry-picked via PR.

Choose a reason for hiding this comment

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

ok ok, I will create a cherry-pick PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So there won't be any rebase meaning this PR is ok'ish and should be valid for the master. If there is any update removal of description needed following this patch it should be removed with PR. Am I understanding this correctly?

Choose a reason for hiding this comment

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

I would remove changes related to DUCR and DDCR from this PR. As they will be automatically picked up once the Velero CRDs are updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will they? This is the bases manifest for the top level CSV so I don't think we have any automagic which is updating this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean it's not the config/crd/bases/*.yaml it's the config/manifests/bases/*.yaml. The first one is for the description in the config object and those are automatically picked from the CRDs, however the second one living n the config/manifests/bases/*.yaml applies to our top level operator and as such it's not automatically synced from any other resources.

Copy link
Member

Choose a reason for hiding this comment

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

As they will be automatically picked up once the Velero CRDs are updated.

The description wouldn't be picked up automatically.

The operator-sdk bundle generation only automate Name, Version, Kind into bundle/CSV.

	for _, defKey := range defKeys {
		if owned, ownedExists := descMap[defKey]; ownedExists {
			ownedDescs = append(ownedDescs, owned)
		} else {
			ownedDescs = append(ownedDescs, operatorsv1alpha1.CRDDescription{
				Name:    defKey.Name,
				Version: defKey.Version,
				Kind:    defKey.Kind,
			})
		}
	}

https://redhat-internal.slack.com/archives/C0144ECKUJ0/p1698773258374849

Still have to put in description manually and phase/specDescriptors if desired.

@mpryc
Copy link
Contributor Author

mpryc commented Nov 1, 2023

/retest

@@ -289,8 +328,46 @@ spec:
displayName: Conditions
path: conditions
version: v1alpha1
- kind: DataUpload
- description: DataUpload represents a data upload of a volume snapshot. There
Copy link
Member

Choose a reason for hiding this comment

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

Please add the descriptions that got merged upstream: https://github.com/vmware-tanzu/velero/pull/7028/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

Adds missing descriptions to the following CRDs:

 - DataDownload
 - DataUpload
 - VolumeSnapshotBackup
 - VolumeSnapshotRestore
 - CloudStorage

Signed-off-by: Michal Pryc <mpryc@redhat.com>
Copy link

openshift-ci bot commented Nov 1, 2023

@mpryc: 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.

@weshayutin
Copy link
Contributor

Screenshot from 2023-11-02 07-23-36

Definitions are present, and look right to me.

Copy link
Member

@kaovilai kaovilai left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@kaovilai kaovilai left a comment

Choose a reason for hiding this comment

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

Sounds like we're ok adding ignored descriptions because it's easier in the moment. Follow up to remove perhaps.

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2023
Copy link

openshift-ci bot commented Nov 2, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kaovilai, mpryc, sseago, weshayutin

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kaovilai
Copy link
Member

kaovilai commented Nov 2, 2023

/lgtm

@weshayutin weshayutin merged commit ad0c242 into openshift:master Nov 2, 2023
15 of 16 checks passed
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 2, 2023
@mpryc
Copy link
Contributor Author

mpryc commented Nov 2, 2023

VSB and VSR need to be removed from the master branch. They are unnecessary in OADP 1.3+, but they also don't pose any harm. In the absence of the VSB and VSR in the CRD's / OCP APIs, the description will not be displayed, so adding extra description won't create any issues. For OADP 1.2, this will be a straightforward backport from the master branch because it contains VSB and VSR, which are required in OADP 1.2.

mpryc added a commit to mpryc/oadp-operator that referenced this pull request Nov 3, 2023
…t#1200)

Adds missing descriptions to the following CRDs:

 - DataDownload
 - DataUpload
 - VolumeSnapshotBackup
 - VolumeSnapshotRestore
 - CloudStorage

Signed-off-by: Michal Pryc <mpryc@redhat.com>
openshift-ci bot pushed a commit that referenced this pull request Nov 3, 2023
…1213)

Adds missing descriptions to the following CRDs:

 - DataDownload
 - DataUpload
 - VolumeSnapshotBackup
 - VolumeSnapshotRestore
 - CloudStorage

Signed-off-by: Michal Pryc <mpryc@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants