-
Notifications
You must be signed in to change notification settings - Fork 103
chore: bump helm and observabiilty versions #201
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,7 @@ | |
chart_name = 'nginx-ingress' | ||
chart_version = config.get('chart_version') | ||
if not chart_version: | ||
chart_version = '0.13.0' | ||
chart_version = '0.15.0' | ||
helm_repo_name = config.get('helm_repo_name') | ||
if not helm_repo_name: | ||
helm_repo_name = 'nginx-stable' | ||
|
@@ -31,7 +31,7 @@ | |
nginx_repository = "nginx/nginx-ingress" | ||
nginx_tag = config.get('nginx_tag') | ||
if not nginx_tag: | ||
nginx_tag = "2.2.0" | ||
nginx_tag = "2.4.0" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this version number correlate to? I don't see it in the version numbers modified in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are two paths for the NGINX IC deployment; one is in the mainline where we do everything in Python - this is the "just pull from the repo w/ the JWT" logic, and for that we need the nginx image tag (latest is 2.4.0). If you look at the NGINX version that's the most recent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the clarification. Ideally this lives in config somewhere but let's put that on #140 |
||
nginx_plus_flag = config.get_bool('nginx_plus_flag') | ||
if not nginx_plus_flag: | ||
nginx_plus_flag = False | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary to adjust in this PR, but in my opinion we should check that the user has specified versions up front and crash with a helpful error message before anything is created. Defaults help with the "plug and play" ability of the example but I think we can have the defaults encoded in the
.example
as above and not hidden in a python script.Thoughts on this? Do you think it would be tough to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we crashed here, would we wind up with any half-created state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely, if we crash out things are really pear-shaped.
The long-term plan - and I think there may be an issue with it; if not I will add one - is to remove the fallbacks in the code for versions and force them all through the config. This requires that we make sure we tell the users how to add these values, and we probably should provide a "just give me all the known working values" option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like we are in agreement but we don't need to hold up this chore PR on it. I'll call this another notch to add to #140