Skip to content

fix targets and build args from clap v4 update #333

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sedlund
Copy link

@sedlund sedlund commented Jul 10, 2025

Disclaimer: this was 'vibe' 🤗 patched with LLM, I know very little of rust.

this fixes usage after the clap v4 update.

it removes --targets and allows --target to be repeated as is how I understand clap v4 wants to work. (having both was confusing and redundant anyway)

to easily specify multiple targets utilize shell brace expansion:

--target=.#{host1.profile1,host2.profile3} etc...

Everything seems to work for me so far. YMMV

#325

@weriomat
Copy link
Contributor

You should probably update the interface.json accordingly

@sedlund
Copy link
Author

sedlund commented Jul 10, 2025

Thanks. I updated the test to use --target.
I didn't see anything specific in interface.json that needs to change. 🧐

@weriomat
Copy link
Contributor

Sorry you are right, there is nothing to be changed there.
Regardless in my opinion this introduces a regression.
I can no longer specify a target via deploy -d -s .#test but I need to deploy -d -s --target .#test.
I can no longer specify multiple targets, but I need to invoke --target multiple times.
Fixing your problem can be done with only removing the group = "deploy" from target and targets.
Maybe we should use custom Validation or no validation at all?

@sedlund
Copy link
Author

sedlund commented Jul 15, 2025

I appreciate your feedback.

I can no longer specify a target via deploy -d -s .#test

This was unintentional.

Although, having 3 ways to specify hosts to deploy is redundant in the code and mental space. It makes the tool less approachable. And in my opinion, what allowed this breakage from not being caught originally.

I suggest having one way to pass targets in the future, however that may be (while supporting brace expansion).

I can no longer specify multiple targets, but I need to invoke --target multiple times

Shell brace expansion makes this better than not using them in most cases. This was possible before.

For example, with the original --targets option:

--targets myFlake#hostAlpha.alice myFlake#hostAlpha.bob myFlake#hostBravo.alice myFlake#hostBravo.bob myFlake#hostCharlie.alice myFlake#hostCharlie.bob

becomes:

--target=myFlake#host{Alpha,Bravo,Charlie}.{alice,bob}

Fixing your problem can be done with only removing the group = "deploy" from target and targets.
Maybe we should use custom Validation or no validation at all?

For clarity, the issues with deploy-rs HEAD as well as in stable nixos-25.05:

  1. Cannot specify multiple targets.
  2. Cannot pass nix build args.

This PR allows a ref to patch with to resolve these (my goal).

I don't have time to continue without concrete suggestions for improvement. If something has been merged to resolve, I will close this.

Thanks again.

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