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

feat(k8s): use pod spec fields in tasks and tests #2165

Merged
merged 2 commits into from Feb 3, 2021

Conversation

thsig
Copy link
Collaborator

@thsig thsig commented Dec 10, 2020

What this PR does / why we need it:

For helm and kubernetes modules, we now use all pod spec fields from the service resource when running the task/test (excluding a fixed blacklist of pod spec fields).

This allows tasks/tests to e.g. use custom service accounts, node selectors etc.

Also refactored / broke up the runAndCopy function.

Which issue(s) this PR fixes:

Fixes #2118.

Special notes for reviewer:

What do we think of the blacklist as it is here? Do we want to add/remove any fields?

@@ -41,7 +41,7 @@ resources: {}
# cpu: 100m
# memory: 128Mi

nodeSelector: {}
nodeSelector: { diskType: ssd }
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may be why the tests are failing. The Pod cannot be scheduled.

@edvald
Copy link
Collaborator

edvald commented Jan 5, 2021

This all seems to make sense @thsig, but there is that test timing out. I had a suspicion as to why, see comment above.

This function was getting too long to easily read.
@thsig thsig force-pushed the pod-runner-config-improvements branch 2 times, most recently from 272a9de to 1bd446c Compare February 3, 2021 09:43
For `helm` and `kubernetes` modules, we now use most pod spec fields from
the service resource when running the task/test (based on a fixed
whitelist of pod spec fields).

This allows tasks/tests to e.g. use custom service accounts, node
selectors etc.
@thsig thsig force-pushed the pod-runner-config-improvements branch from 1bd446c to e9c1cc7 Compare February 3, 2021 10:06
@thsig
Copy link
Collaborator Author

thsig commented Feb 3, 2021

This should be good to go now.

I've replaced the blacklist with a whitelist of pod spec fields, and it also looks like the tests are now fine (since I used a different field than nodeSelector to test the pod spec logic).

@edvald edvald merged commit ce1e8ed into master Feb 3, 2021
@edvald edvald deleted the pod-runner-config-improvements branch February 3, 2021 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE]: expose service account in tests
2 participants