Service ID lookup to include fallback to environment variable #320

Integralist · 2021-06-23T16:13:02Z

Problem: a Service ID must be explicitly set on each CLI invocation using the --service-id flag which is tedious.
Solution: add an env var lookup between the flag and file lookup options.
Notes: to avoid having to duplicate the updated flag description (which is where I wanted to highlight to the user the lookup options) I've moved the flag to a separate function that each command calls.

Addresses: #301

Integralist

@triblondon hopefully this helps...

Integralist · 2021-06-23T16:14:47Z

pkg/backend/create.go

@@ -35,7 +35,7 @@ func NewCreateCommand(parent cmd.Registerer, globals *config.Data) *CreateComman
 	c.manifest.File.SetOutput(c.Globals.Output)
 	c.manifest.File.Read(manifest.Filename)
 	c.CmdClause = parent.Command("create", "Create a backend on a Fastly service version").Alias("add")
-	c.CmdClause.Flag("service-id", "Service ID").Short('s').StringVar(&c.manifest.Flag.ServiceID)
+	c.RegisterServiceIDFlag(&c.manifest.Flag.ServiceID)


Example of new register function.

Integralist · 2021-06-23T16:15:02Z

pkg/cmd/flags.go

@@ -16,6 +16,14 @@ import (
 	"github.com/fastly/kingpin"
 )

+// RegisterServiceIDFlag defines a --service-id flag that will attempt to


Implementation of new register function.

Integralist · 2021-06-23T16:15:27Z

pkg/compute/manifest/manifest.go

@@ -80,6 +84,10 @@ func (d *Data) ServiceID() (string, Source) {
 		return d.Flag.ServiceID, SourceFlag
 	}

+	if sid := os.Getenv(env.ServiceID); sid != "" {


New env var lookup option.

Integralist · 2021-06-23T16:15:50Z

pkg/compute/manifest/manifest_test.go

@@ -159,3 +160,42 @@ func TestManifestPrepend(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+func TestDataServiceID(t *testing.T) {


Tests for the lookup behaviour.

Integralist · 2021-06-23T16:17:36Z

pkg/env/env.go

@@ -10,4 +10,7 @@ const (

 	// Endpoint is the env var we look in for the API endpoint.
 	Endpoint = "FASTLY_API_ENDPOINT"
+
+	// ServiceID is the env var we look in for the required Service ID.
+	ServiceID = "FASTLY_SERVICE_ID"


The new env var.

peterbourgon · 2021-06-28T11:02:31Z

pkg/stats/historical.go

-	c.CmdClause.Flag("service-id", "Service ID").Short('s').Required().StringVar(&c.manifest.Flag.ServiceID)
+	c.RegisterServiceIDFlag(&c.manifest.Flag.ServiceID)


This represents a change in behavior — a previously Required flag is no longer required. It's not a... huge? problem, but it does have an IMO noticeable negative effect on user experience. If a command or subcommand requires a service ID, and I forget to provide one by any means, it's much better to get an error message from the CLI telling me I omitted a required parameter than from the API (or whatever) telling me my request was invalid.

I understand that adding the fastly.toml option makes it tricky, but IMO validating required flags/params at the CLI layer is actually reasonably important...

it's much better to get an error message from the CLI telling me I omitted a required parameter than from the API (or whatever) telling me my request was invalid.

It's not the API telling you the Service ID is missing, but the CLI.

In each command's Exec function we call a function which subsequently triggers:

cli/pkg/cmd/cmd.go

Lines 125 to 128 in a869c58

serviceID, source := opts.Manifest.ServiceID()

if source == manifest.SourceUndefined {

return serviceID, serviceVersion, errors.ErrNoServiceID

}

The logic for the ServiceID function was updated as part of this PR to include an env var lookup:

cli/pkg/compute/manifest/manifest.go

Lines 81 to 96 in a869c58

// ServiceID yields a ServiceID.

func (d *Data) ServiceID() (string, Source) {

if d.Flag.ServiceID != "" {

return d.Flag.ServiceID, SourceFlag

}

if sid := os.Getenv(env.ServiceID); sid != "" {

return sid, SourceEnv

}

if d.File.ServiceID != "" {

return d.File.ServiceID, SourceFile

}

return "", SourceUndefined

}

.

The --service-id is not required in the sense that you aren't required to set the flag if you've got either the appropriate environment variable set, or you have a fastly.toml file.

The problem is that even when using something like Envar to enable a failover to an environment variable (if set by the user), the help output would (unless I'm mistaken) still indicate that the flag itself was required, and that's not strictly speaking true (the Service ID is what's required, but how you provide it is flexible).

This is made harder due to the additional need to fallback to the fastly.toml manifest file.

An alternative, would be to move the fallback logic from our CLI into our fork of Kingpin so that we implement something similar to Envar but which worked specifically for our use case. For example implementing a Fallback chained function like:

- c.CmdClause.Flag("service-id", "Service ID").Short('s').Envar(env.ServiceID).Required().StringVar(&c.Input.ServiceID) + c.CmdClause.Flag("service-id", "Service ID").Short('s').Fallback(...).Required().StringVar(&c.Input.ServiceID)

...where Fallback() excepted a slice of func types that could be executed in order. That way you still get the help output to show --service-id is required, and then you could omit the flag and have it failover to one of the alternatives. I still don't like the fact that it might confuse users who see the help output tell them the flag is required and so they don't question it and feel obliged to always provide the flag.

It would be nice to signify that a 'value' is required without coupling it to a flag. AFAIK this sort of thing would be communicated via man cli but as we're not generating a manual we have to rely on the Kingpin parser to indicate what data is required on a per command basis. So having it indicated via a flag is easier in that sense.

I see I see, so the Required bit in the clause is specifically expressing the semantic of: the literal flag must be provided. That's no good. It would be better if we could catch that during some kind of validation phase, before we got into the command code itself, but IMO that's "nice to have" rather than "important". Thanks for the detailed explanation 👍

An alternative . . .

Sure! An additional alternative — based on my intuition and not verified to actually be viable — might be to leverage the Pre-whatever hooks of the CmdClause to do this and perhaps additional validation as well.

might be to leverage the Pre-whatever hooks of the CmdClause to do this and perhaps additional validation as well.

ah ok, I'm still very green when it comes to Kingpin so there could well be something for me to explore there 👍🏻

Looks like from a brief investigation that none of the PreActions are able to execute before the Required() validation (once the command has a flag defined as required that validation completes first). So at this point I don't think I want to spend too much more time on this. Would be good to revisit but only when it's more of a priority.

Ah, no, I agree you can't use Required. I was suggesting to drop all the Requireds (as you have done) and use the PreAction "stage" as a way to perform validation of flags, rather than in the command function itself. But again I haven't verified this will work and I don't think it's critical!! Just a suggestion for the great and unknowable future ⛵

* failover to env var * move manifest read up a line

* Service ID lookup to include fallback to environment variable (#320) * failover to env var * move manifest read up a line * fix snippet list * add new test scenarios for snippet describe * fix snippet update * ignore vendor directory * fix code to use go go-fastly 3.7.1 * update to latest go-fastly version 3.8.0

Integralist added the enhancement New feature or request label Jun 23, 2021

Integralist requested review from a team and triblondon and removed request for a team June 23, 2021 16:13

Integralist commented Jun 23, 2021

View reviewed changes

Integralist added 2 commits June 23, 2021 17:54

failover to env var

beb8947

move manifest read up a line

a869c58

Integralist force-pushed the integralist/service-id-env branch from 0ccec4e to a869c58 Compare June 23, 2021 16:55

Integralist requested a review from kailan June 23, 2021 16:56

triblondon approved these changes Jun 25, 2021

View reviewed changes

Integralist merged commit 4e2dbff into main Jun 28, 2021

Integralist deleted the integralist/service-id-env branch June 28, 2021 08:19

Integralist mentioned this pull request Jun 28, 2021

Support FASTLY_SERVICE_ID environment variable #306

Closed

peterbourgon reviewed Jun 28, 2021

View reviewed changes

Integralist added a commit that referenced this pull request Jun 28, 2021

Service ID lookup to include fallback to environment variable (#320)

a3fa138

* failover to env var * move manifest read up a line

Integralist added a commit that referenced this pull request Jun 28, 2021

Service ID lookup to include fallback to environment variable (#320)

89e1a3a

* failover to env var * move manifest read up a line

Integralist added a commit that referenced this pull request Jun 28, 2021

Service ID lookup to include fallback to environment variable (#320)

402c4b3

* failover to env var * move manifest read up a line

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Service ID lookup to include fallback to environment variable #320

Service ID lookup to include fallback to environment variable #320

Integralist commented Jun 23, 2021 •

edited

Integralist left a comment

Integralist Jun 23, 2021

Integralist Jun 23, 2021

Integralist Jun 23, 2021

Integralist Jun 23, 2021

Integralist Jun 23, 2021

peterbourgon Jun 28, 2021

peterbourgon Jun 28, 2021

Integralist Jun 28, 2021

peterbourgon Jun 28, 2021

Integralist Jun 28, 2021

Integralist Jun 28, 2021

peterbourgon Jun 28, 2021

		c.CmdClause.Flag("service-id", "Service ID").Short('s').Required().StringVar(&c.manifest.Flag.ServiceID)
		c.RegisterServiceIDFlag(&c.manifest.Flag.ServiceID)

	serviceID, source := opts.Manifest.ServiceID()
	if source == manifest.SourceUndefined {
	return serviceID, serviceVersion, errors.ErrNoServiceID
	}

	// ServiceID yields a ServiceID.
	func (d *Data) ServiceID() (string, Source) {
	if d.Flag.ServiceID != "" {
	return d.Flag.ServiceID, SourceFlag
	}

	if sid := os.Getenv(env.ServiceID); sid != "" {
	return sid, SourceEnv
	}

	if d.File.ServiceID != "" {
	return d.File.ServiceID, SourceFile
	}

	return "", SourceUndefined
	}

Service ID lookup to include fallback to environment variable #320

Service ID lookup to include fallback to environment variable #320

Conversation

Integralist commented Jun 23, 2021 • edited

Integralist 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

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

Integralist commented Jun 23, 2021 •

edited