-
Notifications
You must be signed in to change notification settings - Fork 897
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
Consolidate existing server setting CLI tools #19848
Consolidate existing server setting CLI tools #19848
Conversation
1549157
to
a320678
Compare
The thing about this approach is it's a net +3 lines in the existing script and we get to delete the old script. I don't see any feature we lose. The only downside is we aren't setting it at a zone/region level, so you need to do it for any new servers but that was the case for the old script. We should link to #19747 and have @evertmulder review this to ensure it handles the existing use cases. Also, is the issue you were trying to fix in #19441 also a problem with this PR's approach? |
a320678
to
6ec0126
Compare
@evertmulder any chance you might be able to address #19848 (comment) please? |
@miq-bot add_label enhancement |
Why does this set it server by server as opposed to setting it at the region level once? |
Because I liked option two better: #19747 (comment) |
I don't like option 2 specifically because it doesn't account for new servers. The only con there that seems to make sense is that you have to deal with servers that have the value already overwritten, but that is handled already via the magic settings keywords EDIT: The magic value is |
It's replacing
Are you able to edit/view settings for region/zone in the UI/api? For example, if you blasted a value for a region/zone and wanted to confirm it in the UI/api, I'm not sure if that's easy to do. I know it works in the backend but if it's not in the UI/api, it might make sense to do a more equivalent replacement script while we determine what's missing in the region/zone settings work. |
Yes. You can view and edit the settings at any level. API can also do it at any level. |
TMYK. Cool. I'll have to review it with @d-m-u @yrudman as I think he implemented some of it. I'm not sure how to view settings at the region/zone level. Before this PR, we had 2 scripts saving server based settings:
After this PR, we have most if not all of the former's features by adding 3 lines to the latter. I question if we should first deprecate the former script but nevertheless, it's a surgical replacement. We can implement blasting region/zone settings later on to solve the problem of having to update settings to new servers but my hope was first to remove the nearly duplicate script first. Also, region/zone settings isn't a direct replacement of the former script so I thought this would be easier to take in steps. |
6ec0126
to
3499127
Compare
Yes it's probably all going to be wrong, but we have to start somewhere
3499127
to
c60d50c
Compare
Checked commit d-m-u@c60d50c with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.20.0, and yamllint tools/configure_server_settings.rb
|
Maybe let's rename this PR to "Consolidate existing server setting CLI tools". This PR's intention was to de-duplicate our existing scripts down to one tool to set server settings. It doesn't prevent us from adding zone/region setting support. |
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.
Merging. This PR consolidates 2 scripts that set server settings into 1 by adding 3 whole lines to the existing script. While region/zone settings are possible in the future, it's outside the scope of this PR and far more complicated than 3 lines of code.
@miq-bot add_label jansa/yes |
…tings_pass_one Consolidate existing server setting CLI tools (cherry picked from commit 7f454fc)
Jansa backport details:
|
We don't have current functionality for setting a server setting once for all servers.
It's probably all going to be wrong, but we have to start somewhere.
I'll link the issue when I'm more sure that this is the right approach which honestly might not be ever — as Joe said, the downside of this is that it requires that we push out this server setting change for each new server added to an environment.