Skip to content

solved issue #388 adding the restartPolicy and removing the write permissions check for the packs volumes #407

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
Closed
wants to merge 2 commits into from
Closed

solved issue #388 adding the restartPolicy and removing the write permissions check for the packs volumes #407

wants to merge 2 commits into from

Conversation

@cayohollanda
Copy link

@cayohollanda cayohollanda commented Mar 19, 2024

No description provided.

@pull-request-size pull-request-size bot added the size/S PR that changes 10-29 lines. Very easy to review. label Mar 19, 2024
@CLAassistant
Copy link

CLAassistant commented Mar 19, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

cognifloyd
cognifloyd requested changes Apr 11, 2024
View reviewed changes
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.

I'm not ok with removing the error message and instructions on how to resolve issues with this.

Do you still see these errors now that #403 has been merged? What error messages are you seeing?

cognifloyd
cognifloyd reviewed Apr 11, 2024
View reviewed changes
templates/jobs.yaml
readOnly: false
# TODO: Find out default resource limits for this specific job (#5)
#resources:
restartPolicy: OnFailure
Copy link
Member

@cognifloyd cognifloyd Apr 11, 2024

Choose a reason for hiding this comment

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

Suggested change
restartPolicy: OnFailure

This was added in #403. You can see it below on line 584 below.

stackstorm-k8s/templates/jobs.yaml

Lines 582 to 586 in fabbea9

{{- end }}
# st2canary job does not support extra_volumes. Let us know if you need this.
restartPolicy: Never
{{- if $.Values.dnsPolicy }}
dnsPolicy: {{ $.Values.dnsPolicy }}

Copy link

@fdrab fdrab Apr 11, 2024

Choose a reason for hiding this comment

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

one more thing required for this to not fail is to remove the 2 leading whitespaces before FAIL on line 557. unless someone does it before me, I'll submit a PR once my company approves me signing of the CLA

Copy link
Member

@cognifloyd cognifloyd Apr 11, 2024

Choose a reason for hiding this comment

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

Cool. I have a fix for that. I'll push a PR in a few minutes.

Copy link
Member

@cognifloyd cognifloyd Apr 11, 2024

Choose a reason for hiding this comment

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

Fix pushed in #411

@cognifloyd cognifloyd mentioned this pull request Apr 11, 2024
Merged
@cognifloyd cognifloyd added bug Something isn't working K8s labels Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cognifloyd cognifloyd cognifloyd requested changes

+1 more reviewer

@fdrab fdrab fdrab left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working K8s size/S PR that changes 10-29 lines. Very easy to review.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cayohollanda @CLAassistant @cognifloyd @fdrab