-
Notifications
You must be signed in to change notification settings - Fork 882
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: rh_subscription org value #5390
Conversation
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.
LGTM, thanks for delving into this
Looks good to me, thank you for cleaning this up! |
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.
Thanks for reporting and addressing this!
@@ -2478,8 +2478,8 @@ | |||
"description": "The activation key to use. Must be used with ``org``. Should not be used with ``username`` or ``password``" | |||
}, | |||
"org": { | |||
"type": "integer", | |||
"description": "The organization number to use. Must be used with ``activation-key``. Should not be used with ``username`` or ``password``" | |||
"type": "string", |
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.
As the value of this is ultimately going to be passed as string to subscription-manager
and in order to maintain backwards compatibility, I think we should maintain both types as valid and mark integer
as deprecated.
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.
Thank you for the feedback. I will address this in a new commit!
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.
@pneigel-ca any updates on this?
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.
@holmanb No updates. If someone can handle in a new PR, I'm welcome to it. I haven't had the time to familiarize myself with the deprecation syntax and rebase with the changes to documentation.
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.
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.
@pneigel-ca Is [email protected] your preferred email for the commit message?
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.
@holmanb Thanks for checking in - I didn't configure my email properly in git 😄
[email protected]
can be used.
Update
org
type frominteger
tostring
, updated unit test that referencesorg
.Fixes GH-5382
Proposed Commit Message
Additional Context
The original issue identified schema errors for values that were determined to be appropriate using underlying tools.
Test Steps
Provide sample configuration for
rh_subscription
using astring
for the value oforg
and it should be free of schema errors when validated, ie:cloud-init schema --system
Checklist
Merge type