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

onboarding key generator package rename as it does not implies what it does #2452

Merged
merged 1 commit into from
Mar 3, 2024
Merged

onboarding key generator package rename as it does not implies what it does #2452

merged 1 commit into from
Mar 3, 2024

Conversation

rchikatw
Copy link
Contributor

@rchikatw rchikatw commented Feb 8, 2024 •
edited
Loading

  • renaming the package for the onboarding job as it does not imply what it does. Renamed to onboarding-validation-keys-generator

@rchikatw rchikatw force-pushed the onboarding branch 3 times, most recently from be7ae0f to e265a58 Compare February 12, 2024 08:35
@rchikatw
Copy link
Contributor Author

@leelavg @nb-ohad please review.

nb-ohad
nb-ohad requested changes Feb 12, 2024
View reviewed changes
controllers/storagecluster/provider_server.go Outdated Show resolved Hide resolved
controllers/storagecluster/provider_server.go Show resolved Hide resolved
controllers/storagecluster/provider_server_test.go Outdated Show resolved Hide resolved
deploy/ocs-operator/manifests/onboarding-secret-generator-binding.yaml Outdated Show resolved Hide resolved
deploy/ocs-operator/manifests/onboarding-secret-generator-role.yaml Outdated Show resolved Hide resolved
deploy/ocs-operator/manifests/onboarding-secret-generator-sa.yaml Outdated Show resolved Hide resolved
rbac/onboarding-secret-generator-binding.yaml Outdated Show resolved Hide resolved
rbac/onboarding-secret-generator-role.yaml Outdated Show resolved Hide resolved
rbac/onboarding-secret-generator-sa.yaml Outdated Show resolved Hide resolved
rotate-onboarding-keys/main.go Outdated Show resolved Hide resolved
jarrpa
jarrpa requested changes Feb 12, 2024
View reviewed changes
Copy link
Member

@jarrpa jarrpa left a comment

Choose a reason for hiding this comment

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

I'm in general agreement with Ohad. All the renaming changes have nothing to do with what this PR claims to be doing, and most of them don't make a whole lot of sense per the project's naming conventions. Please move them to a separate PR.

rotate-onboarding-keys/main.go Outdated Show resolved Hide resolved
@rchikatw rchikatw force-pushed the onboarding branch 2 times, most recently from aaab7ab to e79ba81 Compare February 14, 2024 04:53
@rchikatw rchikatw changed the title changing native client to runtime client onboarding key generator package rename as it does not implies what it does Feb 14, 2024
@rchikatw rchikatw force-pushed the onboarding branch 3 times, most recently from e595046 to bd4d104 Compare February 14, 2024 05:16
nb-ohad
nb-ohad requested changes Feb 20, 2024
View reviewed changes
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
controllers/storagecluster/provider_server.go Show resolved Hide resolved
@rchikatw
renaming the package for the onboarding key generator job
8893c73 
renaming the package for the onboarding key generator job
as it does not imply what it does

Signed-off-by: rchikatw <[email protected]>
@nb-ohad
Copy link
Contributor

nb-ohad commented Mar 3, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nb-ohad, rchikatw

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 Mar 3, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 2225e7f into red-hat-storage:main Mar 3, 2024
11 checks passed
@leelavg
Copy link
Contributor

leelavg commented Mar 3, 2024

@rchikatw pls make sure that you inform build team about Dockerfile changes.

@jarrpa
Copy link
Member

jarrpa commented Mar 3, 2024

Hah, I had an earlier review comment that didn't get sent... might as well at least actually publish it.

I don't see why there needs to be a change to the directory name at all, it just looks like introducing needless build changes for the sake of being more verbose. And, as it was, it seemed fine that the directory name itself was kinda vague since the artifacts produced from it are an implementation detail.

@rchikatw
Copy link
Contributor Author

rchikatw commented Mar 4, 2024

pls make sure that you inform build team about Dockerfile changes.

Sure...Thanks for pointing that out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leelavg leelavg leelavg left review comments

@nb-ohad nb-ohad nb-ohad requested changes

@jarrpa jarrpa jarrpa requested changes

Assignees

@jarrpa jarrpa

@nb-ohad nb-ohad

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.
4 participants
@rchikatw @nb-ohad @leelavg @jarrpa