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

helm: add cli argument instanceid #4666

Merged
merged 1 commit into from
Jul 1, 2024
Merged

helm: add cli argument instanceid #4666

merged 1 commit into from
Jul 1, 2024

Conversation

zerotens
Copy link
Contributor

@zerotens zerotens commented Jun 5, 2024

Describe what this PR does

This PR adds the existing cli argument --instanceid as an configurable helm value instanceID.
This allows configuring the instance id parameter of Ceph CSI without manually changing the deployment or helm chart.

Is there anything that requires special attention

Is the change backward compatible?

Yes

Are there concerns around backward compatibility?

No, the default values in code have not been changed but are now overrideable via helm and cli arguments.

Checklist:

  • Commit Message Formatting: Commit titles and messages follow
    guidelines in the developer
    guide    .
  • Reviewed the developer guide on Submitting a Pull
    Request
  • Pending release
    notes
    updated with breaking and/or notable changes for the next major release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.
Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

  • /retest ci/centos/<job-name>: retest the <job-name> after unrelated
    failure (please report the failure too!)

@mergify mergify bot added the component/deployment Helm chart, kubernetes templates and configuration Issues/PRs label Jun 5, 2024
@zerotens zerotens mentioned this pull request Jun 5, 2024
Merged
Rakshith-R
Rakshith-R reviewed Jun 6, 2024
View reviewed changes
Copy link
Contributor

@Rakshith-R Rakshith-R left a comment

Choose a reason for hiding this comment

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

Please take a look. There's a lint failure
https://github.com/ceph/ceph-csi/actions/runs/9388846826/job/25883583707?pr=4666

a new live is required in both values.yaml

@zerotens
Copy link
Contributor Author

Please take a look. There's a lint failure https://github.com/ceph/ceph-csi/actions/runs/9388846826/job/25883583707?pr=4666

a new live is required in both values.yaml

The new line at the end of the files values.yaml should be added now.

Rakshith-R
Rakshith-R reviewed Jun 11, 2024
View reviewed changes
Copy link
Contributor

@Rakshith-R Rakshith-R left a comment

Choose a reason for hiding this comment

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

Can you please mention the new config in docs https://github.com/ceph/ceph-csi/tree/devel/charts/ceph-csi-rbd#configuration similar for cephfs
Sorry for not noticing this earlier.

@Rakshith-R
Copy link
Contributor

Can you please mention the new config in docs https://github.com/ceph/ceph-csi/tree/devel/charts/ceph-csi-rbd#configuration similar for cephfs Sorry for not noticing this earlier.

Please also add a point in PendingReleaseNotes.
https://github.com/ceph/ceph-csi/blob/devel/PendingReleaseNotes.md

@zerotens zerotens force-pushed the helm-instanceid branch 2 times, most recently from fcce186 to a639b8e Compare June 14, 2024 17:53
nixpanic
nixpanic previously approved these changes Jun 18, 2024
View reviewed changes
@nixpanic nixpanic requested review from Rakshith-R and a team June 18, 2024 07:41
Madhu-1
Madhu-1 previously approved these changes Jun 18, 2024
View reviewed changes
@Madhu-1
Copy link
Collaborator

Madhu-1 commented Jun 18, 2024

@zerotens can you please check CI failures?

@mergify mergify bot dismissed stale reviews from Madhu-1 and nixpanic June 21, 2024 11:35

Pull request has been modified.

@zerotens zerotens requested review from Madhu-1 and nixpanic June 21, 2024 11:53
iPraveenParihar
iPraveenParihar approved these changes Jun 27, 2024
View reviewed changes
Copy link
Contributor

@iPraveenParihar iPraveenParihar left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@Rakshith-R
Copy link
Contributor

@Mergifyio queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Jun 28, 2024 •
edited
Loading

queue

🛑 The pull request has been removed from the queue default

The pull request can't be updated.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@zerotens
Copy link
Contributor Author

Unable to update: user zerotens is unknown.
Please make sure zerotens has logged in Mergify dashboard.

Done.

@iPraveenParihar
Copy link
Contributor

@Mergifyio refresh

@mergify Mergify
Copy link
Contributor

mergify bot commented Jul 1, 2024

refresh

✅ Pull request refreshed

@iPraveenParihar
Copy link
Contributor

@Mergifyio queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Jul 1, 2024 •
edited
Loading

queue

🛑 The pull request has been removed from the queue default

The pull request can't be updated.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Jul 1, 2024
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.29

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Jul 1, 2024
@Madhu-1
Copy link
Collaborator

Madhu-1 commented Jul 1, 2024

@Mergifyio rebase

@zerotens @Madhu-1
helm: add cli argument instanceid
4c6f513 
Signed-off-by: Andreas <zerotens@users.noreply.github.com>
@Madhu-1 Madhu-1 force-pushed the helm-instanceid branch from b14d1b9 to 4c6f513 Compare July 1, 2024 11:26
@mergify Mergify
Copy link
Contributor

mergify bot commented Jul 1, 2024

rebase

✅ Branch has been successfully rebased

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Jul 1, 2024

@Mergifyio queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Jul 1, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 1f192ac

@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Jul 1, 2024
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.30

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Jul 1, 2024
@mergify mergify bot merged commit 1f192ac into ceph:devel Jul 1, 2024
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iPraveenParihar iPraveenParihar iPraveenParihar approved these changes

@Madhu-1 Madhu-1 Madhu-1 approved these changes

@Rakshith-R Rakshith-R Awaiting requested review from Rakshith-R

@nixpanic nixpanic Awaiting requested review from nixpanic

Assignees
No one assigned
Labels
component/deployment Helm chart, kubernetes templates and configuration Issues/PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zerotens @Rakshith-R @Madhu-1 @iPraveenParihar @ceph-csi-bot @nixpanic