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

NO-JIRA: azure-disk, azure-file sync code de-duplication #303

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

NO-JIRA: azure-disk, azure-file sync code de-duplication #303

wants to merge 2 commits into from

Conversation

stephenfin
Copy link
Contributor

Both drivers provides syncCloudConfigGuest and syncCloudConfigStandalone functions which were nearly identical. Identical save for the location that the resulting config maps were saved to: Client.GuestNamespace for the former and Client.ControlPlaneNamespace for the latter. However, the docs for Clients.ControlPlaneNamespace say:

Namespace where to install CSI driver control plane. On standalone cluster, it's the same as the guest namespace,
i.e. openshift-cluster-csi-drivers.

This makes the difference between the two functions meaningless and we can instead use syncCloudConfigGuest for both HCP and Standalone deployments. Do just this, renaming the method to drop the Guest suffix.

@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 18, 2024
@openshift-ci-robot
Copy link

@stephenfin: This pull request explicitly references no jira issue.

In response to this:

Both drivers provides syncCloudConfigGuest and syncCloudConfigStandalone functions which were nearly identical. Identical save for the location that the resulting config maps were saved to: Client.GuestNamespace for the former and Client.ControlPlaneNamespace for the latter. However, the docs for Clients.ControlPlaneNamespace say:

Namespace where to install CSI driver control plane. On standalone cluster, it's the same as the guest namespace,
i.e. openshift-cluster-csi-drivers.

This makes the difference between the two functions meaningless and we can instead use syncCloudConfigGuest for both HCP and Standalone deployments. Do just this, renaming the method to drop the Guest suffix.

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot requested review from gnufied and mpatlasov October 18, 2024 12:32
@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 27, 2024
@stephenfin stephenfin force-pushed the azure-disk-sync-cleanup branch from 8bfceed to 24b107a Compare October 31, 2024 14:51
@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, 2024
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2024
@jsafrane
Copy link
Contributor

jsafrane commented Dec 9, 2024

sorry about a late review, it looks good. Can you please rebase?
/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 9, 2024
@stephenfin stephenfin force-pushed the azure-disk-sync-cleanup branch from 24b107a to ffed239 Compare December 9, 2024 17:39
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2024
@stephenfin
Copy link
Contributor Author

sorry about a late review, it looks good. Can you please rebase?

Done 👍

@stephenfin
azure-disk: Remove duplicate config map syncer
a67502c 
The syncCloudConfigGuest and syncCloudConfigStandalone functions were
nearly identical, save for the location that the resulting config maps
were saved to: Client.GuestNamespace for the former and
Client.ControlPlaneNamespace for the latter. However, the docs for
Clients.ControlPlaneNamespace say:

  Namespace where to install CSI driver control plane. On standalone
  cluster, it's the same as the guest namespace i.e.
  openshift-cluster-csi-drivers.

So we can use syncCloudConfigGuest for both HCP and Standalone
deployments. Do just this, renaming the method to drop the Guest suffix.

Signed-off-by: Stephen Finucane <[email protected]>
@stephenfin
azure-file: Remove duplicate config map syncer
ec1c052 
We did this for azure-disk previously. Now do it for azure-file. The
same rationale applies as before.

Signed-off-by: Stephen Finucane <[email protected]>
@stephenfin stephenfin force-pushed the azure-disk-sync-cleanup branch from ffed239 to ec1c052 Compare January 16, 2025 12:31
@jsafrane
Copy link
Contributor

/retest

@jsafrane
Copy link
Contributor

/lgtm
/approve
/retest-required

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

openshift-ci bot commented Feb 11, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, stephenfin

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
Copy link
Contributor

openshift-ci bot commented Feb 11, 2025 •
edited
Loading

@stephenfin: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-openstack-cinder-csi 8bfceed link true /test e2e-openstack-cinder-csi
ci/prow/e2e-openstack-manila-csi 24b107a link true /test e2e-openstack-manila-csi
ci/prow/hypershift-e2e-openstack-csi-manila 24b107a link true /test hypershift-e2e-openstack-csi-manila
ci/prow/hypershift-e2e-openstack-csi-cinder 24b107a link true /test hypershift-e2e-openstack-csi-cinder
ci/prow/smb-operator-e2e-extended ffed239 link false /test smb-operator-e2e-extended
ci/prow/e2e-aws-csi-extended ffed239 link false /test e2e-aws-csi-extended
ci/prow/smb-win2022-operator-e2e ffed239 link false /test smb-win2022-operator-e2e
ci/prow/e2e-azurestack-csi ec1c052 link false /test e2e-azurestack-csi
ci/prow/smb-operator-e2e ec1c052 link false /test smb-operator-e2e

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-sigs/prow repository. I understand the commands that are listed here.

@stephenfin
Copy link
Contributor Author

/retest-required

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

@gnufied gnufied Awaiting requested review from gnufied

@mpatlasov mpatlasov Awaiting requested review from mpatlasov

Assignees

@jsafrane jsafrane

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stephenfin @openshift-ci-robot @jsafrane @openshift-merge-robot