Extract http flags from consul/command pkg #259

lkysow · 2020-05-07T22:48:18Z

Extract flags from consul/command into our own package: subcommand/flags. Remove the flags we weren't using, that leaves:

-http-addr
- Not used anywhere but we use CONSUL_HTTP_ADDR everywhere so this flag matches. We do use it in our tests.
-token
- Not used anywhere but we use CONSUL_HTTP_TOKEN in a lot of locations so this flag matches.
-token-file
- Used in mesh gateways and lifecycle sidecars
-ca-file
- Used by get-auto-encrypt-ca command. CONSUL_CACERT is used by almost all of our components.
-ca-path
- This isn't used by anything currently but I can see us needing this in the future to support intermediate certs.
-tls-server-name
- Used by get-auto-encrypt-ca

Testing: Use lkysow/consul-k8s-dev:may27-flags. Test with all of our components, they'll crash if a flag isn't supported. Note, I've tested this myself.

global:
  imageK8S: lkysow/consul-k8s-dev:may27-flags
  image: consul:beta
  name: consul
  tls:
    enabled: true
    autoEncryptEnabled: true
  federation:
    enabled: true
  acls:
    manageSystemACLs: true
connectInject:
  enabled: true
syncCatalog:
  enabled: true
meshGateway:
  enabled: true

subcommand/inject-connect/command.go

subcommand/lifecycle-sidecar/command_test.go

lkysow · 2020-05-07T22:58:08Z

@hashicorp/consul-eco-platform this is a draft of what it looks like to remove our dependency on consul/command.

ishustava

Sorry for taking so long to review this, Luke!

This looks really good! I left a couple of comments/questions inline.

go.mod

ishustava · 2020-06-24T21:24:04Z

subcommand/flags/flag_duration_value.go

+// This was done so we don't depend on internal Consul implementation.
+
+// DurationValue provides a flag value that's aware if it has been set.
+type DurationValue struct {


This type is currently only used by the sync-catalog command. I've also found that you could just use time.Duration and the flags would parse correctly (see this example in the lifecycle-sidecar command). I'm wondering if we can replace the type of the flagConsulWritePeriod with time.Duration, then maybe we don't need this at all?

I have tried it out quickly, and it seems to be working (branch). This I think would also be fine to refactor afterward as it seems like it's expanding the scope of this task.

I think it's fine to include, thanks for doing the leg work. Done.

ishustava · 2020-06-24T21:28:43Z

subcommand/flags/flag_map_value.go

+
+// FlagMapValue is a flag implementation used to provide key=value semantics
+// multiple times.
+type FlagMapValue map[string]string


I couldn't find how this type is used?

ishustava · 2020-06-24T21:29:46Z

subcommand/flags/flag_slice_value.go

+
+import "strings"
+
+// Taken from https://github.com/hashicorp/consul/blob/master/command/flags/flag_slice_value.go


Could you make all the links tied to a specific git sha? If this code is moved or deleted, then we'd lose this reference.

good call, done!

subcommand/flags/http.go

ishustava · 2020-06-24T21:34:17Z

subcommand/flags/usage.go

+	"io"
+	"strings"
+
+	text "github.com/kr/text"


Do we need to alias it?

no, was just copied. removed.

ishustava · 2020-06-24T21:38:03Z

subcommand/flags/usage.go

+	Flags *flag.FlagSet
+}
+
+func (u *Usager) String() string {


Do we need to test this 😬 ?

I've added a test!

This decouples us from an internal Consul package. We've removed flags that we aren't using.

ishustava

Looks good!! thanks so much for making the changes 🎉

go.mod

ishustava · 2020-07-07T20:54:03Z

subcommand/flags/usage.go

+	Flags *flag.FlagSet
+}
+
+func (u *Usager) String() string {


Run `helm test` in acceptance tests

lkysow force-pushed the generate-fed-secret branch from e6286a1 to 82aebfd Compare May 7, 2020 22:55

lkysow commented May 7, 2020

View reviewed changes

subcommand/inject-connect/command.go Outdated Show resolved Hide resolved

subcommand/lifecycle-sidecar/command_test.go Show resolved Hide resolved

adilyse added the needs-investigation label May 14, 2020

lkysow force-pushed the generate-fed-secret branch from 82aebfd to bfa7f46 Compare May 27, 2020 17:24

lkysow removed the needs-investigation label May 27, 2020

lkysow requested a review from a team May 27, 2020 17:26

lkysow marked this pull request as ready for review May 27, 2020 17:26

adilyse added the area/infrastructure Related to builds, tests, dependencies, etc. of consul-k8s label Jun 24, 2020

ishustava reviewed Jun 24, 2020

View reviewed changes

lkysow added 2 commits July 7, 2020 13:26

Extract http flags from consul/command pkg

74fa670

This decouples us from an internal Consul package. We've removed flags that we aren't using.

Code review feedback

3c87e52

lkysow force-pushed the generate-fed-secret branch from bfa7f46 to 3c87e52 Compare July 7, 2020 20:46

lkysow requested a review from ishustava July 7, 2020 20:47

ishustava approved these changes Jul 7, 2020

View reviewed changes

go.mod Show resolved Hide resolved

subcommand/flags/usage.go

Flags *flag.FlagSet

}

func (u *Usager) String() string {

Copy link

Contributor

ishustava Jul 7, 2020

Choose a reason for hiding this comment

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

🙏

lkysow merged commit a520d4c into master Jul 7, 2020

lkysow deleted the generate-fed-secret branch July 7, 2020 22:53

ndhanushkodi pushed a commit to ndhanushkodi/consul-k8s that referenced this pull request Jul 9, 2021

Merge pull request hashicorp#259 from hashicorp/helm-test-acceptance

57f9b85

Run `helm test` in acceptance tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract http flags from consul/command pkg #259

Extract http flags from consul/command pkg #259

lkysow commented May 7, 2020 •

edited

Loading

lkysow commented May 7, 2020

ishustava left a comment

ishustava Jun 24, 2020

lkysow Jul 7, 2020

ishustava Jun 24, 2020

lkysow Jul 7, 2020

ishustava Jun 24, 2020

lkysow Jul 7, 2020

ishustava Jun 24, 2020

lkysow Jul 7, 2020

ishustava Jun 24, 2020

lkysow Jul 7, 2020

ishustava Jul 7, 2020

ishustava left a comment

ishustava Jul 7, 2020


		import "strings"

		// Taken from https://github.com/hashicorp/consul/blob/master/command/flags/flag_slice_value.go

Extract http flags from consul/command pkg #259

Extract http flags from consul/command pkg #259

Conversation

lkysow commented May 7, 2020 • edited Loading

lkysow commented May 7, 2020

ishustava 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

ishustava left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkysow commented May 7, 2020 •

edited

Loading