Emit notice when unnecessary feature flags are used

[eliottwiener-gridunity]

What existing functionality needs improvement?

I've found Earthly's feature flags to be a useful way to try out experimental features. However, over time my Earthfile's VERSION line became cluttered with feature flags. At some point, I looked at the documentation again, and realized that most of the flags I had been using were no longer necessary, as the features had been accepted and integrated into the release as non-experimental features. I was thinking that it would be nice if Earthly notified the user if their Earthfile has feature flags that are no longer necessary.

EXAMPLE:

Let's say I have an Earthfile like:

VERSION --use-copy-link 0.7
FROM scratch

The feature corresponding to --use-copy-link was accepted for 0.7, and so the --use-copy-link flag is unnecessary in this example Earthfile.

Expected Behavior

Upon invocation, Earthly would emit a notice to the user, informing them that the feature flag is unnecessary. This message would be informational only, and would not affect Earthly's execution otherwise.

Using the note about COMMAND being renamed to FUNCTION in 0.8 as an example, this message might appear in the output like so:

$ earthly +base
 Init 🚀
——————————————————————————————————————————————

buildkitd | Found buildkit daemon as docker container (earthly-buildkitd)

 Build 🔧
——————————————————————————————————————————————
Note that the feature flag --use-copy-link is not necessary for version 0.7, since the feature was enabled by default. You may remove this flag from your Earthfile.

[alexcb]

I like the idea; it doesn't seem that difficult to implement either.

we have

earthly/features/features.go

Lines 193 to 196 in c48f19f

	parsedArgs, err := flagutil.ParseArgsWithValueModifierAndOptions("VERSION", &ftrs, version.Args, instrumentVersion, goflags.PassDoubleDash|goflags.PassAfterNonOption)
	if err != nil {
	return nil, false, err
	}

which parses the initial VERSION --flag --otherflag ... X.Y string,

then the corresponding parts that sets the bools to true:

earthly/features/features.go

Lines 256 to 266 in c48f19f

	if versionAtLeast(ftrs, 0, 8) {
	ftrs.NoNetwork = true
	ftrs.ArgScopeSet = true
	ftrs.UseDockerIgnore = true
	ftrs.PassArgs = true
	ftrs.GlobalCache = true
	ftrs.CachePersistOption = true
	ftrs.GitRefs = true
	ftrs.UseVisitedUpfrontHashCollection = true
	ftrs.UseFunctionKeyword = true
	}

all we would have to do is check to see if these bools are already true and raise a warning if they are.

the naive approach would be to do, for example:

                if ftrs.NoNetwork {
                    issue warning
                }
		ftrs.NoNetwork = true

but that seems rather tedious (and error-prone) and should be avoided if possible -- maybe we can use go generate to come up with a different approach for this. The good news is this is all contained in this single Get method, which limits scope.

[alexcb]

we have a lot of open issues at the moment (and we're trying to stay focused on bug fixes) -- no promise on a timeline for us to get to it, but PRs for this would be most welcomed.

[danqixu]

@alexcb , hi I would like to work on this issue, could u assign it to me?

[alexcb]

PRs for this would be most welcomed

@danqixu this is still an open issue -- go for it. However I unfortunately can't offer much support with this task due to competing priorities.