-
Notifications
You must be signed in to change notification settings - Fork 819
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
Improve Schema For cc_set_passwords (SC-1105) #1517
Conversation
Just a drive by comment... |
Sounds good. No problem, I'll add that to the new key as well. |
FYI, https://bugs.launchpad.net/cloud-init/+bug/1979065 . Not sure if it makes sense to address this here (I actually think it's probably better not to), but since you're in the middle of modifying some of this, you may want to incorporate this too. |
I can do that in a follow up PR while this module is fresh in my brain. |
39801cc
to
523d9d7
Compare
I just added some tests to verify that duplicate users in input don't break anything in a new way. This is arguably invalid input, but since we don't currently check for it I just wanted to ensure it didn't lead to brokenness while merging the two supported inputs together. |
plist = util.get_cfg_option_str(chfg, "list", plist) | ||
if plist: | ||
plist = plist.splitlines() | ||
multiline = util.get_cfg_option_str(chfg, "list") |
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.
The change here should be a noop change and was made for typing reasons.
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.
+1. Really nice UX schema addition.
I left you a minor nit. The indentation in that line was made with tabs instead of spaces.
Co-authored-by: Alberto Contreras <aciba90@gmail.com>
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 much delayed review. but looks great. Just a missing deprecated: true I think and minor text changes.
Do what you will with the unittest suggestions.
Non-blocking comments, you can merge once deprecated: true is in schema.
Co-authored-by: Chad Smith <chad.smith@canonical.com>
Co-authored-by: Chad Smith <chad.smith@canonical.com>
Co-authored-by: Chad Smith <chad.smith@canonical.com>
@blackboxsw - Great feedback, thanks! I committed your suggestions verbatim.
It looks like adding the deprecation key causes schemavalidation errors to be raised under strict validation in some tests. You already gave approval, but given this change wasn't discussed previously I'll wait until you've had a chance to review that part (last 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.
I've seen this come up as a concern multiple times from users. I'd also personally like to see this improved.
One remaining item to check before merging:
what happens when identical users are in both<- No longer a todo item: tests have been added that verify current behavior is preserved with the current change.list:
andusers:
? Would it be better to catch this with a runtime exception or allow any error to bubble up fromchpasswd
?