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

feat: supportAllowWildCard and AllowNotFound #7437

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

TomChv
Copy link
Member

@TomChv TomChv commented May 22, 2024

Fixes #3338

⚠️ I'm hitting an issue locally when I try to test this feature, it seems my new params are not taken into account.
I'm doing a push to see if I hit the same behaviour in the CI (maybe it's due to some reloading or cache in the API)

Fixes dagger#3338

Signed-off-by: Tom Chauveau <tom@epitech.eu>
@TomChv TomChv added the Engine label May 22, 2024
@TomChv TomChv requested a review from vito May 22, 2024 10:24
@TomChv TomChv self-assigned this May 22, 2024
jedevc and others added 4 commits May 22, 2024 13:04
When we removed mage, we removed the easy helpers that allowed
generating/linting/etc on all SDKs. That meant that making API changes
became much more fiddly.

This patch restores the "all" group, but this time directly in the
dagger module. One day, we might remove this with a script, but we don't
have that right now.

Signed-off-by: Justin Chadwell <me@jedevc.com>
This can be done by attaching an optional signal argument to the
existing `stop` method (which will wait until the process terminates
after sending `stop`), or using the new `signal` method (which returns
immediately).

Signed-off-by: Justin Chadwell <me@jedevc.com>
Without this, the metadata would not be passed for a required enum arg.

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
@TomChv TomChv requested review from a team as code owners May 22, 2024 11:33
Comment on lines +58 to +60
ArgDoc("path", `Location of the file to remove (e.g., "/file.txt").`).
ArgDoc("allowWildCard", `Allow wildcards in the path (e.g., "*.txt").`).
ArgDoc("allowNotFound", `Allow the operation to not fail if the directory does not exist.`),
Copy link
Member

@jedevc jedevc May 22, 2024

Choose a reason for hiding this comment

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

FYI - we would also want this on container. Side note (and not sure how to resolve this), it feels weird that we have so much repetition with the 4 methods here - Container/File.withoutFile/withoutDirectory.

@TomChv
Copy link
Member Author

TomChv commented May 22, 2024

Based in this discussion: https://discord.com/channels/707636530424053791/1240714480275689574/1242807626723491921 we're going to hold this PR for the next release and find a better API design.

There's still that strange issue where my parameter are not considered, even in the issue, I'm not sure what's the root cause.

/cc @vito @sipsma @helderco

@TomChv TomChv marked this pull request as draft May 22, 2024 12:00
path = filepath.Join(dir.Dir, path)
err = dir.SetState(ctx, st.File(llb.Rm(path, llb.WithAllowWildcard(true), llb.WithAllowNotFound(true))))
Copy link
Member

@jedevc jedevc May 22, 2024

Choose a reason for hiding this comment

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

Huh. I didn't know we defaulted to this.

This feels slightly inconsistent with other stuff - see another discussion on magic processing in #7173. A bit strange that some paths can be globs/wildcards/expansions and some aren't.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes definitely, I think that's why we wanted to add the parameter.

@@ -521,6 +521,13 @@ func TestDirectoryWithoutDirectoryWithoutFile(t *testing.T) {
require.NoError(t, err)
require.Equal(t, []string{"some-dir", "some-file"}, entries)

// Test without dir with allow not found = false
_, err = dir1.
WithoutDirectory("non-existant", dagger.DirectoryWithoutDirectoryOpts{AllowNotFound: false}).
Copy link
Member

Choose a reason for hiding this comment

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

@TomChv this is your issue.

In go, the empty struct defaults to false - therefore, we have the API constraint today that all boolean options must default to false.

What you need is to invert the option, to something like MustExist: true, so that the default is false.

@samalba samalba added area/engine About dagger core engine and removed Engine labels Jun 4, 2024
Copy link
Contributor

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/engine About dagger core engine kind/stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

withoutDirectory and withoutFile should support AllowWildcard and AllowNotFound
3 participants