Extend autocompletion to be build arg aware #1330

alexcb · 2021-10-20T23:11:02Z

Earthly will now look at ARG commands within a target
and make suggestions based on them. e.g.

earthly +update-buildkit -<tab><tab>

will suggest

--BUILDKIT_GIT_SHA, --BUILDKIT_GIT_BRANCH

Additionally earthly will no longer suggest global flags after
a target was given (which was incorrect behaviour).

Signed-off-by: Alex Couture-Beil alex@earthly.dev

andrewsykim · 2021-10-22T15:32:53Z

earthfile2llb/earthfile2llb.go

-	targets := make([]string, 0, len(ef.Targets))
-	for _, target := range ef.Targets {
+	targets := make([]string, 0, len(bc.Earthfile.Targets))
+	for _, target := range bc.Earthfile.Targets {


What was the reason for passing in the resolver here? It seems like we're passing in the resolver here to get the Earthfile which was also possible before by using ast.Parse

I'm hoping to extend auto-completion to also work on remote targets in a future PR, which will require the resolver.

andrewsykim · 2021-10-22T15:33:10Z

earthfile2llb/earthfile2llb.go

+
+// GetTargetArgs returns a list of build arguments for a specificed target
+func GetTargetArgs(ctx context.Context, resolver *buildcontext.Resolver, gwClient gwclient.Client, target domain.Target) ([]string, error) {
+	bc, err := resolver.Resolve(ctx, gwClient, target)


Same comment here about passing in resolver instead of using ast.Parse

andrewsykim

Still reviewing this PR but had some initial questions

andrewsykim · 2021-10-25T15:38:23Z

autocomplete/complete.go

+		if strings.HasSuffix(targetToParse, "+") {
+			targetToParse += "base"
+		}
+		target, err := domain.ParseTarget(targetToParse)


Does domain.ParseTarget expect a full target name but here we could be passing in just a prefix?

It does, without the target name ParseTarget returns an error since it's not a target.

I think I'm just missing something here, but if we return err here wouldn't that return early here instead of expanding the potentials below?

let's assume we're expanding on ./path+, If we call domain.ParseTarget(targetToParse) that will return an error as it is not a valid target.

So prior to calling that we call: if strings.HasSuffix(targetToParse, "+") {, and then pretend the user is expanding on ./path+base instead which will allow domain.ParseTarget to return successfully.

When we call earthfile2llb.GetTargets, we pass it a target (because that's what the resolver expects); however we never actually look at the target name -- we only look at the path to the Earthfile. The reason I want to change it from a string path and to a target, is to allow us to (one-day) expand on a remote target.

What if targetsToParse (or prefix) is ./path+foo (where full target is foobar), in this case we wouldn't be passing +base to ParseTarget right?

how about I change // GetTargets returns a list of targets from an Earthfile. -> // GetTargets returns a list of targets from an Earthfile. Note that the target name of domain.Target is ignored and doesn't matter?

I agree it's weird, it's just to work around the fact that the resolver expects a target.

I guess it's not ignoring it completely because it uses it for LocalDirs and other metadata, but my initial instinct is to assume it's some sort of filter used in GetTargets

how about I change // GetTargets returns a list of targets from an Earthfile. -> // GetTargets returns a list of targets from an Earthfile. Note that the target name of domain.Target is ignored and doesn't matter?

I agree it's weird, it's just to work around the fact that the resolver expects a target.

Sounds good to me

// GetTargets returns a list of targets from an Earthfile. // Note that the passed in domain.Target's target name is ignored (only the reference to the Earthfile is used)

lgtm, thanks!

autocomplete/complete.go

tests/autocompletion/Earthfile

andrewsykim · 2021-10-25T16:14:47Z

tests/autocompletion/Earthfile

+    RUN diff expected actual
+
+    RUN echo "--city=" > expected
+    RUN COMP_LINE="earthly +othertargetwithargs --ci" COMP_POINT=33 earthly > actual


Can we also add a test case here where autocomplete is called in the middle of a build arg value? I'm assuming in that case there should be no potentials returned right?

If we set it to COMP_POINT=8, we will truncate it to earthly and autocomplete based on that.

Currently (even without this PR) if you type earthly --intera<tab><tab>ctive it will give us earthly --interactive ctive. If we want to fix that bug, I would rather open a new issue (and PR) for that.

Currently (even without this PR) if you type earthly --interactive it will give us earthly --interactive ctive. If we want to fix that bug, I would rather open a new issue (and PR) for that.

Ah interseting. I was referring to build arg values, not args to earthly. Would this bug still apply in that case? For example earthly +target --MY_ARG=foo<tab><tab>, which I would assume returns nothing because we can't know what potential values would be.

It applies in all cases:

earthly/autocomplete/complete.go

Line 195 in 6fa0591

compLine = compLine[:compPoint]

andrewsykim

Thanks for taking the time to answer all my questions! :)

Earthly will now look at ARG commands within a target and make suggestions based on them. e.g. earthly +update-buildkit -<tab><tab> will suggest --BUILDKIT_GIT_SHA, --BUILDKIT_GIT_BRANCH Additionally earthly will no longer suggest global flags after a target was given (which was incorrect behaviour). Signed-off-by: Alex Couture-Beil <alex@earthly.dev>

alexcb force-pushed the acb/build-arg-auto-complete branch 3 times, most recently from d7bd274 to d82cd92 Compare October 20, 2021 23:55

alexcb marked this pull request as ready for review October 21, 2021 00:12

alexcb requested review from vladaionescu, dchw and andrewsykim October 21, 2021 00:12

andrewsykim reviewed Oct 22, 2021

View reviewed changes

andrewsykim reviewed Oct 25, 2021

View reviewed changes

alexcb force-pushed the acb/build-arg-auto-complete branch 2 times, most recently from 36791e1 to ef95973 Compare October 25, 2021 17:16

andrewsykim approved these changes Oct 25, 2021

View reviewed changes

alexcb force-pushed the acb/build-arg-auto-complete branch from ef95973 to fb0559b Compare October 25, 2021 17:38

alexcb merged commit 49b8002 into main Oct 25, 2021

alexcb deleted the acb/build-arg-auto-complete branch October 25, 2021 17:55

alexcb mentioned this pull request Oct 26, 2021

Extend bash autocompletion to include buildargs #1240

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend autocompletion to be build arg aware #1330

Extend autocompletion to be build arg aware #1330

alexcb commented Oct 20, 2021

andrewsykim Oct 22, 2021

alexcb Oct 22, 2021

andrewsykim Oct 22, 2021

andrewsykim left a comment

andrewsykim Oct 25, 2021

alexcb Oct 25, 2021

andrewsykim Oct 25, 2021

alexcb Oct 25, 2021

andrewsykim Oct 25, 2021

alexcb Oct 25, 2021

andrewsykim Oct 25, 2021

andrewsykim Oct 25, 2021 •

edited

alexcb Oct 25, 2021

andrewsykim Oct 25, 2021

andrewsykim Oct 25, 2021

alexcb Oct 25, 2021

andrewsykim Oct 25, 2021 •

edited

alexcb Oct 25, 2021

andrewsykim left a comment

Extend autocompletion to be build arg aware #1330

Extend autocompletion to be build arg aware #1330

Conversation

alexcb commented Oct 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsykim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsykim Oct 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsykim Oct 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsykim left a comment

Choose a reason for hiding this comment

andrewsykim Oct 25, 2021 •

edited

andrewsykim Oct 25, 2021 •

edited