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

settings.yml support string settings values #20908

Merged
merged 2 commits into from
Jan 6, 2021

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Dec 18, 2020

Overview

The serialization of symbols in our yaml file is giving us issues when transferring the data over json.
this breaks the react code that is transferring it

See issues in ManageIQ/manageiq-api#979

Change

Most of the symbol values we use are used in send or have a to_sym so the storage format does not matter.
This change fixes the one place in our code where we do not have a to_sym

yaml changes

I'm more than happy removing the yaml change, I just wanted us to see the underlying change so we can better visualize this PR.

we are going away from storing symbols in settings.yml

This change ensures that the symbols in the settings.yml can be replaced with strings
and everything will work as it did before
we are updating settings to no longer store symbols.
this makes the settings not ruby specifi
this also makes settings serializable in json as well
@miq-bot
Copy link
Member

miq-bot commented Dec 18, 2020

Checked commits kbrock/manageiq@bca660a~...e49a49f with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@@ -963,13 +963,13 @@
:worker_monitor:
:enforce_resource_constraints: false
:kill_algorithm:
:name: :used_swap_percent_gt_value
:name: used_swap_percent_gt_value
Copy link
Member

Choose a reason for hiding this comment

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

Does the caller not care if it's symbol or string? (same with the other algorithms). Asking because I don't see a corresponding code change like what you did for poll_method.

Copy link
Member

Choose a reason for hiding this comment

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

I just checked, it uses send so it should be ok.

@Fryguy
Copy link
Member

Fryguy commented Jan 4, 2021

I think this will require a data migration for any existing overrides of these fields.

@Fryguy Fryguy self-assigned this Jan 4, 2021
@jrafanie
Copy link
Member

jrafanie commented Jan 4, 2021

I think this will require a data migration for any existing overrides of these fields.

I agree but think we can merge this now and do the migration followup.

@Fryguy
Copy link
Member

Fryguy commented Jan 4, 2021

Agreed, but I don't want us to forget ;)

@jrafanie
Copy link
Member

jrafanie commented Jan 4, 2021

Agreed, but I don't want us to forget ;)

@kbrock can you add a work item to ManageIQ/manageiq-api#979 so we don't forget to fix existing settings.yml related values in customer/user databases as that will break what you're trying to solve in the api.

@kbrock
Copy link
Member Author

kbrock commented Jan 6, 2021

thnx. done. added the TODO line

@Fryguy Fryguy merged commit 65a5229 into ManageIQ:master Jan 6, 2021
@kbrock kbrock deleted the yaml_symbols branch June 4, 2021 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants