Skip to content

refactor: client install CLI DX#10454

Merged
TomChv merged 1 commit into
dagger:mainfrom
TomChv:fix/client-gen-install-spec
May 28, 2025
Merged

refactor: client install CLI DX#10454
TomChv merged 1 commit into
dagger:mainfrom
TomChv:fix/client-gen-install-spec

Conversation

@TomChv
Copy link
Copy Markdown
Member

@TomChv TomChv commented May 22, 2025

@shykes mentioned that the current dagger client install CLI doesn't completely follows the spec described in #9582.

This PR aims to fix that difference:
Before: dagger client install --generator=<generator> <output-path>
Now: dagger client install <generator> <output-path>

@shykes mentionned that the current `dagger client install` CLI doesn't
completely follows the spec described in dagger#9582.

This PR aims to fix that difference:
Before: `dagger client install --generator=<generator> <output-path>`
Now: `dagger client install <generator> <output-path>`

Signed-off-by: Tom Chauveau <tom@dagger.io>
@TomChv TomChv requested a review from jedevc May 22, 2025 11:48
@TomChv TomChv self-assigned this May 22, 2025
@jedevc
Copy link
Copy Markdown
Contributor

jedevc commented May 22, 2025

👀 I actually prefer the current DX - it stylistically matches the --sdk flag to init/develop.

That said, if this is the route we go down, the implementation LGTM.

cc @shykes

@TomChv
Copy link
Copy Markdown
Member Author

TomChv commented May 22, 2025

👀 I actually prefer the current DX - it stylistically matches the --sdk flag to init/develop.

That said, if this is the route we go down, the implementation LGTM.

cc @shykes

Yeah agree with you, waiting for @shykes approval before merging

@shykes
Copy link
Copy Markdown
Contributor

shykes commented May 22, 2025

The reason for the syntax I proposed, is to make the path optional. Most users (me included) don't know intuitively what path to provide. So each generator should pick its own default. I suspect this is not currently supported in the backend. So it's not just a matter of UI bikeshedding: the spec encapsulated a desired backend behavior.

@shykes
Copy link
Copy Markdown
Contributor

shykes commented May 22, 2025

dagger client install GENERATOR [PATH] [-n|--no-generate]

So I want to be able to run:

dagger client install go

And have the go generator figure out where to install it (by default)

@shykes
Copy link
Copy Markdown
Contributor

shykes commented May 22, 2025

(btw this kind of detail is why I insisted that we discuss the spec in the issue, before rushing an implementation)

@TomChv
Copy link
Copy Markdown
Member Author

TomChv commented May 22, 2025

The reason for the syntax I proposed, is to make the path optional.

The path is already optional, the client will be generated in dagger by default if nothing is set, if you have any suggestion for a specific default path on each SDK, I'm happy to implement that too.

(btw this kind of detail is why I insisted that we discuss the spec in the issue, before rushing an implementation)

Happy to chat about it whenever you can

@shykes
Copy link
Copy Markdown
Contributor

shykes commented May 22, 2025

we discussed this live with Tom fyi

@jedevc
Copy link
Copy Markdown
Contributor

jedevc commented May 23, 2025

Sure, but having --generator=go isn't mutually exclusive with having PATH be optional. We could have dagger client install --generator=go? I agree that users may not know what path to put in, but I don't see why that's the same discussion as having a generator flag (even if that flag is required).

Just thinking about ways to reduce friction for users who are already using init/develop today.

@TomChv
Copy link
Copy Markdown
Member Author

TomChv commented May 23, 2025

Sure, but having --generator=go isn't mutually exclusive with having PATH be optional. We could have dagger client install --generator=go? I agree that users may not know what path to put in, but I don't see why that's the same discussion as having a generator flag (even if that flag is required).

Just thinking about ways to reduce friction for users who are already using init/develop today.

Yeah both solutions works, if --generator looks important, we can close that PR, maybe we can ask some early tester of the client generator?

@jedevc
Copy link
Copy Markdown
Contributor

jedevc commented May 27, 2025

Don't want to block on having --generator as a flag, implementation looks good to me if @shykes agrees.

@shykes
Copy link
Copy Markdown
Contributor

shykes commented May 27, 2025

Yeah you're right that the 2 topics are orthogonal @jedevc .

dagger client install go is simpler for users than dagger client install --generator=go. It also follows unix conventions (required arguments should not be flags, also known as "options"). And it's more consistent with dagger install <thing to install>

@TomChv TomChv merged commit a5d1e8b into dagger:main May 28, 2025
56 checks passed
helderco pushed a commit to helderco/dagger that referenced this pull request May 28, 2025
@shykes mentionned that the current `dagger client install` CLI doesn't
completely follows the spec described in dagger#9582.

This PR aims to fix that difference:
Before: `dagger client install --generator=<generator> <output-path>`
Now: `dagger client install <generator> <output-path>`

Signed-off-by: Tom Chauveau <tom@dagger.io>
Ben-Ackerman added a commit to Ben-Ackerman/dagger that referenced this pull request Aug 2, 2025
Ben-Ackerman added a commit to Ben-Ackerman/dagger that referenced this pull request Aug 2, 2025
…o longer used after the refactor on dagger#10454

Signed-off-by: Ben Ackerman <Ben@tacmail.net>
Ben-Ackerman added a commit to Ben-Ackerman/dagger that referenced this pull request Aug 2, 2025
… is no longer used after the refactor on PR dagger#10454

Signed-off-by: Ben Ackerman <Ben@tacmail.net>
TomChv pushed a commit that referenced this pull request Aug 7, 2025
… is no longer used after the refactor on PR #10454 (#10830)

Signed-off-by: Ben Ackerman <Ben@tacmail.net>
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.

3 participants