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

external: change cluster_name to k8s_cluster_name #12811

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

parth-gr
Copy link
Member

@parth-gr parth-gr commented Aug 29, 2023

Description of your changes:
it was confusing if we are calling a ceph cluster name or k8s cluster name, so re-named

Which issue is resolved by this Pull Request:
Resolves #

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Skip Tests for Docs: If this is only a documentation change, add the label skip-ci on the PR.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@parth-gr parth-gr requested a review from travisn August 29, 2023 14:55
Copy link
Contributor

@subhamkrai subhamkrai left a comment

Choose a reason for hiding this comment

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

Canary/Canary test is failing PTAL

@parth-gr
Copy link
Member Author

Canary/Canary test is failing PTAL

Updated

it was confusing if we are calling a ceph cluster name
or k8s cluster name, so re-named

Signed-off-by: parth-gr <paarora@redhat.com>
Copy link
Member

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -45,7 +45,7 @@ python3 create-external-cluster-resources.py --rbd-data-pool-name <pool_name> --
- `--skip-monitoring-endpoint`: (optional) Skip prometheus exporter endpoints, even if they are available. Useful if the prometheus module is not enabled
- `--ceph-conf`: (optional) Provide a Ceph conf file
- `--keyring`: (optional) Path to Ceph keyring file, to be used with `--ceph-conf`
- `--cluster-name`: (optional) Ceph cluster name
- `--k8s-cluster-name`: (optional) Kubernetes cluster name
Copy link
Member

Choose a reason for hiding this comment

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

If someone is still using the --cluster-name flag will they get an error? It will be good to make it obvious that they need to rename the parameter.

Copy link
Member Author

Choose a reason for hiding this comment

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

No there won't be any error,

The script is used at very start just to connect the cluster.

Copy link
Member Author

Choose a reason for hiding this comment

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

Only during upgrade caps they need to take care of it to use new flag,

But which is already updated with the script description

Copy link
Member

Choose a reason for hiding this comment

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

Could the script print an error if --cluster-name is passed? The error can tell them to instead use --k8s-cluster-name. This way, if someone doesn't look at the release notes they can quickly know they have need to rename this parameter.

Copy link
Member Author

Choose a reason for hiding this comment

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

So if we use --cluster-name it will print all the correct flags that can be used

Copy link
Member Author

@parth-gr parth-gr Aug 31, 2023

Choose a reason for hiding this comment

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

Should we mention this in the release notes?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so if they use --cluster-name, the script will already fail, thanks for confirming.
For the upstream release, all the commits are listed in the release notes, so it will already be covered.

@parth-gr parth-gr requested a review from travisn August 30, 2023 16:36
@travisn travisn merged commit a06fd8a into rook:master Aug 31, 2023
47 of 50 checks passed
travisn added a commit that referenced this pull request Aug 31, 2023
external: change cluster_name to k8s_cluster_name (backport #12811)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants