zone-setup cleanup/refactoring#6175
Merged
Merged
Conversation
karencfv
approved these changes
Jul 29, 2024
karencfv
left a comment
Contributor
There was a problem hiding this comment.
Thanks a bunch for taking the time to improve this code, it's looking a whole lot nicer! Just left an optional nit, but otherwise looks great to me!
I do agree that it's best to test out on Madrid before merging, but I'll leave it approved so you can merge once the testing is done.
| }; | ||
| fn add_new_group_for_user(&self) -> Result<(), ExecutionError> { | ||
| if get_group_by_name(&self.group).is_none() { | ||
| execute( |
Contributor
There was a problem hiding this comment.
Oooh! This is much better!
Contributor
Author
|
Tested on
all successfully. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(This is a rebased and expanded version of #6015 now that the self-assembling switch zone has landed.)
I need to make some changes to zone-setup to support Reconfigurator rebalancing boundary NTP zones, and found it a little trickier to work with than expected. I think there were two reasons for this:
I also made a cleanup pass through the new switch zone user setup code; the biggest win there was reusing
illumos_utils::execute()to check for success and build detailed error messages on failure. There are some other minor changes like long flags with different names (AFAICT we only ever use the short flags in SMF manifests, so this shouldn't require other changes) and potential bugfixes (e.g., the switch zone setup code was only correct for 0 or 1 profile, and should now be correct for 2+ if we ever need more than one).While all these should be correct, I want to run this through madrid before merging. I think it's ready for review, though; will update here once I do the madrid test.