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

Deprecate symbol suport for tools/configure_server_settings #21006

Merged
merged 1 commit into from
Feb 3, 2021

Conversation

Fryguy
Copy link
Member

@Fryguy Fryguy commented Feb 2, 2021

In the move away from allowing symbols in the settings (#20908), we should drop the symbol support from this tool as well.

@jrafanie @kbrock Please review.

@jrafanie
Copy link
Member

jrafanie commented Feb 2, 2021

Is there a link to where this was removed? Was it deprecated first?

@Fryguy
Copy link
Member Author

Fryguy commented Feb 2, 2021

#20908 - Updated the OP

Also the more general ManageIQ/manageiq-api#979

I think there still needs to be a database migration for any existing symbols, which is on @kbrock 's radar, I believe.

@jrafanie
Copy link
Member

jrafanie commented Feb 2, 2021

So, I'm guessing it would be hard to deprecate it in settings, or would it? But if so, shouldn't we deprecate the client here before we just drop support for it?

@Fryguy
Copy link
Member Author

Fryguy commented Feb 2, 2021

What's to deprecate? We don't support symbols anymore and all symbols have become strings. Are you suggesting keep the symbol and spit out a deprecation, then just convert to .to_s anyway?

Since it's a tool, I guess I don't really understand the purpose of deprecation?

@jrafanie
Copy link
Member

jrafanie commented Feb 2, 2021

What's to deprecate? We don't support symbols anymore and all symbols have become strings. Are you suggesting keep the symbol and spit out a deprecation, then just convert to .to_s anyway?

Since it's a tool, I guess I don't really understand the purpose of deprecation?

Yeah, but it seems odd to just drop support for -t symbol and suddenly exit with an error to use one of string integer boolean float array. Maybe this is overkill: honor -t symbol but treat it like a string with a warning that symbol support is deprecated as it's now treated like a string anyway? I don't know if anyone ever populated symbol values with this tool. Poll escalate seems like such a unlikely value to be set.

@Fryguy
Copy link
Member Author

Fryguy commented Feb 2, 2021

Yeah...I have no problem deprecating, but the audience for it seems really small if at all.

@Fryguy
Copy link
Member Author

Fryguy commented Feb 2, 2021

@jrafanie Updated.

@Fryguy Fryguy changed the title Drop symbol suport for tools/configure_server_settings Deprecate symbol suport for tools/configure_server_settings Feb 2, 2021
@miq-bot
Copy link
Member

miq-bot commented Feb 2, 2021

Checked commit Fryguy@dd253c9 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 👍

@jrafanie
Copy link
Member

jrafanie commented Feb 3, 2021

Tests are failing with "pg_restore exit code: 1 error was: pg_restore: [archiver] could not open input file "foo/bar/mkfifo": No such file or directory"... should be fixed by #21009... kicking.

@jrafanie jrafanie closed this Feb 3, 2021
@jrafanie jrafanie reopened this Feb 3, 2021
@jrafanie jrafanie merged commit ccf50e7 into ManageIQ:master Feb 3, 2021
@Fryguy Fryguy deleted the drop_symbol_support branch February 3, 2021 22:21
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.

3 participants