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

deploy: add extraArgs for sidecars #3560

Merged
merged 1 commit into from
Jan 5, 2023
Merged

deploy: add extraArgs for sidecars #3560

merged 1 commit into from
Jan 5, 2023

Conversation

Sea-you
Copy link
Contributor

@Sea-you Sea-you commented Nov 29, 2022

Describe what this PR does

  • Add the extraArgs option to both charts for command-line arguments that were not exposed so far.

Is there anything that requires special attention

N/A

Do you have any questions? false

Is the change backward compatible? true

Are there concerns around backward compatibility? false

Related issues

N/A

Future concerns

N/A

@nixpanic nixpanic added the component/deployment Helm chart, kubernetes templates and configuration Issues/PRs label Nov 30, 2022
@nixpanic
Copy link
Member

@Sea-you , many thanks for the PR!

Could you squash all commits into one, and give it a suitable subject and message? The subject should probably be something like deploy: add extraArgs for sidecars or similar (a prefix like deploy: is required). Also do not forget your Signed-off-by in the commit message.

Once that is done, we'll let the CI run and see if there are any remaining issues.

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Nov 30, 2022

Please add any default arguments in any sidecar here https://github.com/ceph/ceph-csi/blob/devel/scripts/install-helm.sh#L172-L182. to make sure current changes works as expected

@Sea-you Sea-you changed the title Helm charts - Add sidecar extraArgs deploy: add extraArgs for sidecars Dec 5, 2022
@humblec
Copy link
Collaborator

humblec commented Dec 8, 2022

@Sea-you can you please address previous review comments ?

@Sea-you
Copy link
Contributor Author

Sea-you commented Dec 8, 2022

@Sea-you can you please address previous review comments ?

yep, I'll push the changes soon :)

@Sea-you
Copy link
Contributor Author

Sea-you commented Dec 13, 2022

@humblec The extra arguments are by default empty in the values.yaml file, so the installation will go through nicely without any changes. Other arguments for CSI sidecars are exposed differently, so unless someone needs to add an extra argument, no other changes are needed as I see. Wdyt?

@nixpanic pushed all fixes :)

nixpanic
nixpanic previously approved these changes Dec 13, 2022
View reviewed changes
humblec
humblec previously approved these changes Dec 13, 2022
View reviewed changes
@mergify mergify bot dismissed stale reviews from nixpanic and humblec December 13, 2022 10:41

Pull request has been modified.

@Sea-you
Copy link
Contributor Author

Sea-you commented Dec 13, 2022

fixed commit body length 😮‍💨

nixpanic
nixpanic previously approved these changes Dec 14, 2022
View reviewed changes
@nixpanic nixpanic requested review from humblec and a team December 14, 2022 16:42
@Madhu-1
Copy link
Collaborator

Madhu-1 commented Dec 21, 2022

./charts/ceph-csi-rbd/values.yaml
213:81 error line too long (105 > 80 characters) (line-length)
227:81 error line too long (102 > 80 characters) (line-length)
238:81 error line too long (111 > 80 characters) (line-length)
247:81 error line too long (138 > 80 characters) (line-length)

CI failure. @Sea-you can you please fix the linter errors?

@mergify mergify bot dismissed nixpanic’s stale review January 5, 2023 10:20

Pull request has been modified.

@Sea-you Sea-you force-pushed the add/helm-sidecar-extraArgs branch 2 times, most recently from cfd173e to 0df8f84 Compare January 5, 2023 10:25
@Sea-you
Copy link
Contributor Author

Sea-you commented Jan 5, 2023

@Madhu-1 @nixpanic PTAL

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Jan 5, 2023

@Sea-you linter is still failing. Please check. you can run the tests on your system like CONTAINER_CMD=docker make containerized-test TARGET=lint-extras

@Sea-you
Copy link
Contributor Author

Sea-you commented Jan 5, 2023

CONTAINER_CMD=docker make containerized-test TARGET=lint-extras

I was testing with the actual yamllint command you use, but forgot to fix in the cephfs values file :) Fixed now.

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

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

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

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

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

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

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

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

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

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

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

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

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

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

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

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

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

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

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

/test ci/centos/upgrade-tests-cephfs

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

/test ci/centos/upgrade-tests-rbd

@mergify mergify bot removed the ok-to-test Label to trigger E2E tests label Jan 5, 2023
@Sea-you @mergify
deploy: add extraArgs for sidecars
3a4a921 
Add the ability to control more arguments for CSI sidecar components.

Signed-off-by: Domonkos Cinke <seayou@gmail.com>
@mergify
Copy link
Contributor

mergify bot commented Jan 5, 2023

⚠️ This pull request got rebased on behalf of a random user of the organization.
This behavior will change on the 1st February 2022, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

@leseb leseb force-pushed the add/helm-sidecar-extraArgs branch from 1d32098 to 3a4a921 Compare January 5, 2023 13:59
@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Jan 5, 2023
@github-actions
Copy link

github-actions bot commented Jan 5, 2023

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

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

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

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

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

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

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

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

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

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

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

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

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

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

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

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

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

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

/test ci/centos/upgrade-tests-cephfs

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

/test ci/centos/upgrade-tests-rbd

@mergify mergify bot merged commit b7b491c into ceph:devel Jan 5, 2023
@mergify mergify bot removed the ok-to-test Label to trigger E2E tests label Jan 5, 2023
@Sea-you Sea-you deleted the add/helm-sidecar-extraArgs branch March 6, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@yati1998 yati1998 yati1998 approved these changes

@nixpanic nixpanic nixpanic left review comments

@humblec humblec Awaiting requested review from humblec

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.

None yet
5 participants
@Sea-you @nixpanic @Madhu-1 @humblec @yati1998