Skip to content

BUILD-284 Use openshift/api and openshift/client-go for clientset, informers, and listers#57

Merged
openshift-merge-robot merged 3 commits intoopenshift:masterfrom
coreydaley:jira_build_284_use_openshift_api
Oct 12, 2021
Merged

BUILD-284 Use openshift/api and openshift/client-go for clientset, informers, and listers#57
openshift-merge-robot merged 3 commits intoopenshift:masterfrom
coreydaley:jira_build_284_use_openshift_api

Conversation

@coreydaley
Copy link
Copy Markdown

@coreydaley coreydaley commented Oct 1, 2021 •
edited
Loading

This pull request vendors in the relocated API (openshift/api) and clientset/informers/listers (openshift/client-go).
I made ONLY the changes that had to be done for this to work, no further reorganization or refactorings were done, so as to make the review easier, as program logic has not changed.

@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 1, 2021
@openshift-ci openshift-ci Bot requested review from gabemontero and otaviof October 1, 2021 17:33
@gabemontero
Copy link
Copy Markdown
Contributor

gabemontero commented Oct 1, 2021

/assign @gabemontero

@gabemontero
Copy link
Copy Markdown
Contributor

gabemontero commented Oct 1, 2021

realizing this is still WIP and the openshift/api PR has not been fully approved/merge, so changes with the API are not out of the question, but the relative simplicity of these changes so far make me smile :-)

@coreydaley coreydaley mentioned this pull request Oct 6, 2021
Closed
@coreydaley
Copy link
Copy Markdown
Author

coreydaley commented Oct 6, 2021

/hold
waiting on openshift/client-go#198

@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 6, 2021
@gabemontero
Copy link
Copy Markdown
Contributor

gabemontero commented Oct 6, 2021

so hopefully a useful heads up @coreydaley ... even with WIP and hold labels

I believe we got "lucky" wrt not needing out of the gate the corrected csi driver name in openshift/api#1019 because we are using string vs. the constant as defined in

csi-driver-shared-resource/cmd/main.go

Line 83 in 7a51ab2

rootCmd.Flags().StringVar(&driverName, "drivername", "csi.sharedresource.openshift.io", "name of the driver")

csi-driver-shared-resource/test/framework/pod.go

Line 42 in 7a51ab2

Driver: "csi.sharedresource.openshift.io",

csi-driver-shared-resource/test/framework/pod.go

Line 72 in 7a51ab2

Driver: "csi.sharedresource.openshift.io",

that said, if / hopefully when openshift/api#1019 merges relatively soon, after vendoring in that commit level hopefully as part of this PR, to also pick up SharedConfigMapReference, we should make a mental note to update those 3 spots with use of the constant in openshift/api

@gabemontero
Copy link
Copy Markdown
Contributor

gabemontero commented Oct 7, 2021 

 INFO[2021-10-06T21:40:00Z] # temporary creation of CRD until it lands in openshift/api, openshift/openshift-apiserver, etc.
oc apply -f ./deploy/0000_10_sharedsecret.crd.yaml
error: the path "./deploy/0000_10_sharedsecret.crd.yaml" does not exist

Makefile disconnect ^^

@coreydaley
Copy link
Copy Markdown
Author

coreydaley commented Oct 7, 2021

/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 7, 2021
@coreydaley coreydaley changed the title [WIP] BUILD-284 Use openshift/api and openshift/client-go for clientset, informers, and listers BUILD-284 Use openshift/api and openshift/client-go for clientset, informers, and listers Oct 7, 2021
@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 7, 2021
@coreydaley
Copy link
Copy Markdown
Author

coreydaley commented Oct 7, 2021

/assign @adambkaplan

@coreydaley
Copy link
Copy Markdown
Author

coreydaley commented Oct 7, 2021

/retest

@gabemontero
Copy link
Copy Markdown
Contributor

gabemontero commented Oct 7, 2021

/approve

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 7, 2021
@coreydaley
Copy link
Copy Markdown
Author

coreydaley commented Oct 11, 2021

/assign @otaviof

gabemontero
gabemontero approved these changes Oct 12, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@gabemontero gabemontero left a comment

Choose a reason for hiding this comment

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

some side / tangential comments but

/lgtm

Comment thread hack/generate-release-yaml.sh

echo -e "\n---\n" >> release.yaml
cat deploy/${FILE} >> release.yaml
cat ${FILE} >> release.yaml
Copy link
Copy Markdown
Contributor

@gabemontero gabemontero Oct 12, 2021

Choose a reason for hiding this comment

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

yeah this most likely will go away once we intergrate with the CSO, but better to keep it sensible in the interim

Comment thread pkg/cache/common.go
func BuildKey(r sharev1alpha1.ResourceReference) string {
return r.Namespace + ":" + r.Name
func BuildKey(namespace, name string) string {
return namespace + ":" + name
Copy link
Copy Markdown
Contributor

@gabemontero gabemontero Oct 12, 2021

Choose a reason for hiding this comment

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

feels like a "non-required" change but OK :-)

Copy link
Copy Markdown
Author

@coreydaley coreydaley Oct 12, 2021

Choose a reason for hiding this comment

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

We don't have a single resource reference type to use for this anymore.

Copy link
Copy Markdown
Contributor

@gabemontero gabemontero Oct 12, 2021

Choose a reason for hiding this comment

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

ah right .... that said, probably should have used the k8s interface that includes GetName and GetNamespace methods, but no biggie

Comment thread pkg/client/client.go
}

func ExecuteSAR(shareName, podNamespace, podName, podSA string, kind sharev1alpha1.ResourceReferenceType) (bool, error) {
func ExecuteSAR(shareName, podNamespace, podName, podSA string, kind consts.ResourceReferenceType) (bool, error) {
Copy link
Copy Markdown
Contributor

@gabemontero gabemontero Oct 12, 2021

Choose a reason for hiding this comment

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

right this came up in the openshift/api review

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Oct 12, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: coreydaley, gabemontero

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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-merge-robot openshift-merge-robot merged commit 5a3c51b into openshift:master Oct 12, 2021
@gabemontero gabemontero mentioned this pull request Oct 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@otaviof otaviof Awaiting requested review from otaviof

1 more reviewer

@gabemontero gabemontero gabemontero approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@otaviof otaviof

@adambkaplan adambkaplan

@gabemontero gabemontero

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@coreydaley @gabemontero @otaviof @adambkaplan @openshift-merge-robot