Skip to content
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

fix: Clear defaultArgs on withEntrypoint by default #6280

Conversation

helderco
Copy link
Contributor

@helderco helderco commented Dec 15, 2023

Fixes #6044

What?

When Container.withEntrypoint is set, it will clear Container.defaultArgs, unless withEntrypoint(args: [...], keepDefaultArgs: true) is set.

Breaking change

May have low impact since in most cases users are probably erasing defaultArgs manually. Only breaks for users that change the entrypoint but depend on the default args remaining the same.

@helderco

This comment was marked as resolved.

@helderco helderco force-pushed the helder/gtm-1109-withentrypoint-should-clear-default-arguments-by-default branch from b647f2f to 3f5643e Compare December 19, 2023 13:26
@helderco helderco changed the title BREAKING: Clear defaultArgs on withEntrypoint by default fix: Clear defaultArgs on withEntrypoint by default Dec 19, 2023
@helderco helderco removed the kind/breaking Breaking Change label Dec 19, 2023
@helderco helderco marked this pull request as ready for review December 19, 2023 13:27
@helderco helderco requested review from a team as code owners December 19, 2023 13:27
@helderco helderco added this to the v0.9.5 milestone Dec 19, 2023
@helderco
Copy link
Contributor Author

helderco commented Dec 19, 2023

Reframing as a fix, since I agree with @jedevc:

I think I read it more of a "fix", which brings the behaviour of dagger's entrypoint+args inline with how users expect it to work with docker/other runtimes.

- Originally posted in Discord

\cc @gerhard

@helderco helderco requested a review from jedevc December 19, 2023 13:30
helderco added a commit to helderco/hello-dagger that referenced this pull request Dec 19, 2023
After dagger/dagger#6280 `WithDefaultArgs` is cleared by default when `WithEntrpoint` is set.

Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
@helderco helderco force-pushed the helder/gtm-1109-withentrypoint-should-clear-default-arguments-by-default branch from 3f5643e to ef50424 Compare December 19, 2023 14:04
@helderco helderco force-pushed the helder/gtm-1109-withentrypoint-should-clear-default-arguments-by-default branch from ef50424 to 2c85160 Compare December 19, 2023 15:53
core/schema/container.go Show resolved Hide resolved
core/integration/container_test.go Outdated Show resolved Hide resolved
@jedevc
Copy link
Member

jedevc commented Dec 20, 2023

LGTM! Not a blocker: but should this behavior also apply to withoutEntrypoint from #6278? Since the entrypoint is changing, the args wouldn't necessarily apply. I'd argue we should also have the same behavior there for consistency.

@helderco
Copy link
Contributor Author

helderco commented Dec 20, 2023

[...] should this behavior also apply to withoutEntrypoint from #6278? Since the entrypoint is changing, the args wouldn't necessarily apply. I'd argue we should also have the same behavior there for consistency.

Yeah, that's a good point!

Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
This partially reverts commit e76aaf92d2e00d6c8ee6059bca514c53c7c047ca. Will make a separate PR for merging after release.

Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
@helderco helderco force-pushed the helder/gtm-1109-withentrypoint-should-clear-default-arguments-by-default branch from d890892 to 36e111b Compare December 20, 2023 11:21
@helderco helderco merged commit b886626 into dagger:main Dec 20, 2023
43 checks passed
@helderco helderco deleted the helder/gtm-1109-withentrypoint-should-clear-default-arguments-by-default branch December 20, 2023 11:41
helderco added a commit to dagger/hello-dagger that referenced this pull request Jan 24, 2024
After dagger/dagger#6280 `WithDefaultArgs` is cleared by default when `WithEntrpoint` is set.

Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
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.

withEntrypoint should clear default arguments (by default)
4 participants