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

feat: applicationset volumes and volumemounts spec #1563

Merged
merged 1 commit into from
Oct 17, 2024
Merged

feat: applicationset volumes and volumemounts spec #1563

merged 1 commit into from
Oct 17, 2024

Conversation

appiepollo14
Copy link
Contributor

What type of PR is this?

/kind enhancement

What does this PR do / why we need it:

To be able to add extra volumes and volumeMounts to the appSet deployment, see related issue.

Have you updated the necessary documentation?

Documentation update is required by this PR.
Documentation has been updated.
Which issue(s) this PR fixes:

Fixes redhat-developer/gitops-operator#620

How to test changes / Special notes to the reviewer:

@appiepollo14 appiepollo14 changed the title Fixes https://github.com/redhat-developer/gitops-operator/issues/620 feat: applicationset volumes and volumemounts spec Oct 13, 2024
@appiepollo14 appiepollo14 force-pushed the feat/appsetcontroller-extra-volume-mounts branch 8 times, most recently from 4ace5a4 to 874e0c2 Compare October 14, 2024 15:09
@appiepollo14
Copy link
Contributor Author

appiepollo14 commented Oct 14, 2024 •
edited
Loading

@svghadi can you please help me with the following questions:
a. Is there a need to write e2e-tests for this quite trivial change. I've written unit tests for it.
b. If e2e-tests are required, I've added a starter, can you explain why it if failing. Check the CI-logs. How is it possible that the modified applicationSet api, is not available in the e2e tests. Is this by design? Can't a api change be accompanied with a new e2e-test in the same PR? Please give me some pointers.

see newly created issue #1566 for running this locally. Would be very handy to be able to test the kuttl tests locally. Would decrease a cycle with 45 mins in this case.

Edit: I've come to the conclusion that I've was using the v1alpha1 api in the e2e-test and changed the v1beta1 api. Which raises my question. Is it OK to only change v1beta1 spec?

@appiepollo14 appiepollo14 force-pushed the feat/appsetcontroller-extra-volume-mounts branch 2 times, most recently from 62d1923 to 7ccf2f1 Compare October 15, 2024 06:00
@appiepollo14 appiepollo14 marked this pull request as ready for review October 15, 2024 06:00
@appiepollo14
Copy link
Contributor Author

@svghadi ready for your approval!

@svghadi
Copy link
Collaborator

svghadi commented Oct 17, 2024

a. Is there a need to write e2e-tests for this quite trivial change. I've written unit tests for it.

Well, it depends on the feature. Personally, I try to add e2e tests for every feature. However, sometimes it can be tricky, so exceptions can be made.

Edit: I've come to the conclusion that I've was using the v1alpha1 api in the e2e-test and changed the v1beta1 api. Which raises my question. Is it OK to only change v1beta1 spec?

Yes. v1alpha1 is deprecated so changes to v1beta1 only are fine.

svghadi
svghadi requested changes Oct 17, 2024
View reviewed changes
Copy link
Collaborator

@svghadi svghadi left a comment

Choose a reason for hiding this comment

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

Left a suggestion, rest all looks good.

Also, it would be good to have a unit or e2e test to check default vols & mounts when additional vols & mounts are not set/removed from ArgoCD CR.

tests/k8s/1-007_validate_volume_mounts/02-assert.yaml Show resolved Hide resolved
@appiepollo14 appiepollo14 force-pushed the feat/appsetcontroller-extra-volume-mounts branch from 7ccf2f1 to 96b7f08 Compare October 17, 2024 06:29
@appiepollo14
Copy link
Contributor Author

Left a suggestion, rest all looks good.

Also, it would be good to have a unit or e2e test to check default vols & mounts when additional vols & mounts are not set/removed from ArgoCD CR.

There already is, each test calling: https://github.com/appiepollo14/argocd-operator/blob/feat/appsetcontroller-extra-volume-mounts/controllers/argocd/applicationset_test.go#L106 checks the default vols * mounts.

@svghadi
Copy link
Collaborator

svghadi commented Oct 17, 2024

Okay. Looks good then. Can you fix the merge conflicts?

@appiepollo14
Fixes redhat-developer/gitops-operator#620
536c09e 
* Adds volumes and volumeMounts to argocd types
* Adds volumes ans volumeMounts to appset deployment and pod
* Updates generated files
* Updates docs
* Adds unit tests
* Adds e2e-test

Signed-off-by: appiepollo14 <[email protected]>
@appiepollo14 appiepollo14 force-pushed the feat/appsetcontroller-extra-volume-mounts branch from 96b7f08 to 536c09e Compare October 17, 2024 10:33
@appiepollo14
Copy link
Contributor Author

Okay. Looks good then. Can you fix the merge conflicts?

Fixed!

svghadi
svghadi approved these changes Oct 17, 2024
View reviewed changes
Copy link
Collaborator

@svghadi svghadi 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 @appiepollo14 for the contribution.

@svghadi svghadi merged commit 8278397 into argoproj-labs:master Oct 17, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svghadi svghadi svghadi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

(Extra)VolumeMount for appset controller
2 participants
@appiepollo14 @svghadi