Skip to content

zone-setup cleanup/refactoring#6175

Merged
jgallagher merged 16 commits into
mainfrom
john/zone-setup-cleanup
Jul 31, 2024
Merged

zone-setup cleanup/refactoring#6175
jgallagher merged 16 commits into
mainfrom
john/zone-setup-cleanup

Conversation

@jgallagher

Copy link
Copy Markdown
Contributor

(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:

  • The use of clap's builder interface instead of derive (addressed in 3708774), which requires a surprising amount of boilerplate and custom parsing functions, most of which are now gone
  • Using Result<(), CmdError::Failure> throughout, when really only main cares about that and everywhere else could use anyhow::Result<()> (mostly addressed in cac26dd); I also tried to make sure we always attach some .context() to errors (the old code did this in some cases but not all)

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.

@jgallagher jgallagher requested review from bnaecker and karencfv July 29, 2024 16:52

@karencfv karencfv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread zone-setup/src/bin/zone-setup.rs
};
fn add_new_group_for_user(&self) -> Result<(), ExecutionError> {
if get_group_by_name(&self.group).is_none() {
execute(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oooh! This is much better!

@jgallagher

Copy link
Copy Markdown
Contributor Author

Tested on london; that did catch one small problem that blocked correct configuration of wicketd (fixed in 788669e). As of that commit, I went through:

  • clean install
  • RSS
  • mupdate

all successfully.

@jgallagher jgallagher merged commit 35a5041 into main Jul 31, 2024
@jgallagher jgallagher deleted the john/zone-setup-cleanup branch July 31, 2024 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants