Skip to content

Conversation

@bmarick
Copy link
Contributor

@bmarick bmarick commented Oct 12, 2022

My database connection required me to provide a certificate trust chain to validate the connection.
I am able to set that certificate in the deployments by using the extra_volumes options in the values file.
However, the Jobs do not have that option currently available.

This is my attempt to enable the extra_volume support to Jobs.
I have tested the changes in my cluster and validated that the volume mounts worked for my use case.

@pull-request-size pull-request-size bot added the size/M PR that changes 30-99 lines. Good size to review. label Oct 12, 2022
@arm4b arm4b added the feature label Oct 12, 2022
@arm4b
Copy link
Member

arm4b commented Oct 12, 2022

Good stuff, +1 for the feature.

Looks like the tests are failing though and need an update too.

@bmarick
Copy link
Contributor Author

bmarick commented Oct 12, 2022

@armab Unit Tests have been fixed can you merge this change?

Copy link

@copart-jafloyd copart-jafloyd left a comment

Choose a reason for hiding this comment

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

This is almost ready to merge - just a few minor suggestions, and then it looks good!

@@ -0,0 +1,47 @@
---
suite: Custom Annotations

Choose a reason for hiding this comment

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

Suggested change
suite: Custom Annotations
suite: Extra Volumes

Could we rename this file to extra_volumes_test.yaml?

Each file tests one feature across all applicable templates. We can add more extra_volumes tests in another PR for all the deployments that use this too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both file and suite name have been updated

values.yaml Outdated
# HTTP_PROXY: http://proxy:1234
## These named secrets (managed outside this chart) will be added to envFrom.
envFromSecrets: []
# mount extra volumes on the st2web pod(s) (primarily useful for k8s-provisioned secrets)

Choose a reason for hiding this comment

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

Suggested change
# mount extra volumes on the st2web pod(s) (primarily useful for k8s-provisioned secrets)
# mount extra volumes on the jobs pods (primarily useful for k8s-provisioned secrets)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment should be updated

@cognifloyd
Copy link
Member

Ooops. @copart-jafloyd is me. I reviewed in the wrong window. Sorry!

@arm4b
Copy link
Member

arm4b commented Oct 12, 2022

Thanks for the reviews @cognifloyd & @copart-jafloyd ! 😸

@bmarick
Copy link
Contributor Author

bmarick commented Oct 13, 2022

@cognifloyd & @copart-jafloyd the changes you have requested have been completed and the tests are passing.
Are there any additional changes you would like to see?

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

LGTM (this time reviewing with the right account 😉)

@cognifloyd cognifloyd merged commit a795f5a into StackStorm:master Oct 13, 2022
@bmarick bmarick deleted the jobsExtraVolumes branch October 13, 2022 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature size/M PR that changes 30-99 lines. Good size to review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants