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

BUILD-353: csi driver, crd name changes #9

Conversation

gabemontero
Copy link
Contributor

the group name was previouisly incorrect, but the mistake happened to land on the choice decided upon (sharedresource vs. storage)

/assign @coreydaley
/assign @adambkaplan

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 28, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabemontero

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2021
@@ -271,7 +271,8 @@ rules:
- apiGroups:
- sharedresource.openshift.io
resources:
- shares
- sharedConfigs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

notice up in line 272 here .... with all the various rename churn, we missed changing this to what is now the "old" value, storage.openshift.io

we go lucky, in that we wanted on sharedresource.openshift.io

@gabemontero
Copy link
Contributor Author

I suspect the e2e-aws-csi-driver will fail until openshift/csi-driver-shared-resource#55 merges

if it doesn't, we may need to investigate whey @adambkaplan @coreydaley

@gabemontero
Copy link
Contributor Author

the failure was

 oc apply -f ./csi-driver-yaml-that-will-live-in-cluster-storage-operator.yaml
The ClusterCSIDriver "csi.sharedresource.openshift.io" is invalid: metadata.name: Unsupported value: "csi.sharedresource.openshift.io": supported values: "ebs.csi.aws.com", "efs.csi.aws.com", "disk.csi.azure.com", "pd.csi.storage.gke.io", "cinder.csi.openstack.org", "csi.vsphere.vmware.com", "manila.csi.openstack.org", "csi.ovirt.org", "csi.kubevirt.io", "csi.shared-resources.openshift.io"
make: *** [deploy] Error 1 

Maybe I missed a spot?

@gabemontero
Copy link
Contributor Author

gabemontero commented Sep 28, 2021

OK it is from openshift/api: ./vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.go: SharedResourcesCSIDriver CSIDriverName = "csi.shared-resources.openshift.io"

so when @coreydaley 's openshift/api openshift/api#979 with the renames land, we vendor the update here, and we go from there

presumably openshift/csi-driver-shared-resource#55 will merge before all that

@otaviof
Copy link
Member

otaviof commented Oct 4, 2021

In general it looks good, I hope the CI issues will solved soon 👍🏼

@gabemontero
Copy link
Contributor Author

gabemontero commented Oct 5, 2021

actually we'll need minimally openshift/api#1019 to merge and vendor that in as part of this PR before we can move forward

I don't think we need @coreydaley to vendor in openshift/api#979 and openshift/api#1019 into the driver repo to get the e2e's to pass from here, given my previous BUILD-353 efforts, but the proof will be in the e2e run

@gabemontero
Copy link
Contributor Author

swell ... go-vet is reporting errors in CI I am not seeing locally

investigating

@gabemontero
Copy link
Contributor Author

gabemontero commented Oct 7, 2021

nuked all of /vendor locally then rebuilt ... I think it added the missing dir's CI's go vet was complaining about

otherwise, may still need to vendor in again after openshift/api#1023 merges (yet another rename spot we missed that @coreydaley just discovered)

@gabemontero
Copy link
Contributor Author

yeah looks like we need openshift/api#1023 merged and vendored in here

@gabemontero
Copy link
Contributor Author

in case openshift/csi-driver-shared-resource#57 has some bearing

/retest

@gabemontero
Copy link
Contributor Author

perhaps this was dependent on promotion / mirroring that took place recently

/retest

@gabemontero
Copy link
Contributor Author

seemingly unrelated cluster install problem this time

/retest

@@ -271,7 +271,8 @@ rules:
- apiGroups:
- sharedresource.openshift.io
resources:
- shares
- sharedConfigs
Copy link
Member

Choose a reason for hiding this comment

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

Should this be sharedconfigmaps?

Suggested change
- sharedConfigs
- sharedconfigmaps

@@ -271,7 +271,8 @@ rules:
- apiGroups:
- sharedresource.openshift.io
resources:
- shares
- sharedConfigs
- sharedSecrets
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be sharedsecrets

Suggested change
- sharedSecrets
- sharedsecrets

@gabemontero
Copy link
Contributor Author

thanks @coreydaley - update pushed / fingers crossed

@@ -271,7 +271,8 @@ rules:
- apiGroups:
- sharedresource.openshift.io
resources:
- shares
- sharedconfigs
Copy link
Member

Choose a reason for hiding this comment

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

sharedconfigmaps

Suggested change
- sharedconfigs
- sharedconfigmaps

Copy link
Member

@coreydaley coreydaley left a comment

Choose a reason for hiding this comment

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

I think it should be sharedconfigmaps

@gabemontero
Copy link
Contributor Author

yep fixed thanks @coreydaley

@coreydaley
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2021
@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@gabemontero
Copy link
Contributor Author

/hold

CI is currently busted https://coreos.slack.com/archives/CEKNRGF25/p1634230288225500

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2021
@coreydaley
Copy link
Member

/retest

1 similar comment
@coreydaley
Copy link
Member

/retest

@gabemontero
Copy link
Contributor Author

Well we are back to

The ClusterCSIDriver "csi.sharedresource.openshift.io" is invalid: metadata.name: Unsupported value: "csi.sharedresource.openshift.io": supported values: "ebs.csi.aws.com", "efs.csi.aws.com", "disk.csi.azure.com", "pd.csi.storage.gke.io", "cinder.csi.openstack.org", "csi.vsphere.vmware.com", "manila.csi.openstack.org", "csi.ovirt.org", "csi.kubevirt.io", "csi.shared-resources.openshift.io"

since I've vendored the rename into this repo @coreydaley that must mean either that

a) we need your openshift/openshift-apiserver PR openshift/openshift-apiserver#244
b) we need the analogous change minimally in cluster storage operator https://github.com/openshift/cluster-storage-operator/blob/master/vendor/github.com/openshift/api/operator/v1/types_csi_cluster_driver.go#L54
c) both a) and b)

@coreydaley
Copy link
Member

/retest

@coreydaley
Copy link
Member

Opened openshift/cluster-storage-operator#224 which will hopefully address this.

@coreydaley
Copy link
Member

/retest
Not sure how long it will take for the newest cluster-storage-operator image to get published, hopefully that fixes this.

@coreydaley
Copy link
Member

/retest
I am not convinced that the last run picked up the latest cluster storage operator image.
If this one fails, then it could be the openshift-apiserver needs to be updated first

…t, but the mistake happened to land on the choice decided upon (sharedresource vs. storage)
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 15, 2021
@coreydaley
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 15, 2021
@gabemontero
Copy link
Contributor Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 15, 2021
@openshift-merge-robot openshift-merge-robot merged commit cbd681f into openshift:master Oct 15, 2021
@gabemontero gabemontero deleted the crd-group-driver-rename-chgs branch October 15, 2021 18:44
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. 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

6 participants