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

fix(account-app) correct default values for 'resources' to avoid YAML parsing error #780

Merged

Conversation

dduportal
Copy link
Contributor

Thanks @olblak for catching the root issue ❤️

This PR fixes an issue when using the default value for resources which should be {} to ensure all YAML parser consider it as null.

Please note that I have a personal preference for commenting out the empty or undefined values on helm charts, but this fix will do.

… parsing error

Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
@dduportal
Copy link
Contributor Author

  • Added unit tests
  • An helmfile diff on the production shows no change (except version meta)

@dduportal dduportal added bug Something isn't working accountapp labels Sep 25, 2023
@dduportal
Copy link
Contributor Author

Self-merging as this PR fixes the updatecli build

@dduportal dduportal merged commit 2b59d9e into jenkins-infra:main Sep 25, 2023
2 checks passed
@dduportal dduportal deleted the fix/account-app/fix-default-values branch September 25, 2023 14:31
@@ -29,8 +29,8 @@ ingress:
# - secretName: accounts-tls
# hosts:
# - accounts.jenkins.io
resources: # We usually recommend not to specify default resources and to leave this as a conscious
# choice for the user. This also increases chances charts run on environments with little
# We usually recommend not to specify default resources and to leave this as a conscious: null # choice for the user. This also increases chances charts run on environments with little

Choose a reason for hiding this comment

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

null or empty object? The comment seems to imply one should change the defaut {} value to null

Also, should this be splited to two lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. The fix with with helm control flow element does not care as the resulting template pipeline is false wether the value is null, empty or undefined.

PR to update the comment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accountapp bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants