VC-35630: Add unit tests to the code that loads config/flags by maelvls · Pull Request #563 · jetstack/jetstack-secure

maelvls · 2024-08-23T21:03:19Z

Stacked on top of #564.

I found that almost none of the "config/flags" code is tested, which made me wary of changing the code without adding more tests. And since I'll have to change this code again for VC-35630, I'd prefer writing some unit tests first.

This PR aims to change no actual logic. The changes you see are the refactoring I had to do to be able to unit test how configuration is loaded.

maelvls · 2024-08-23T21:10:43Z

~~I've just realized that this PR will be very hard to review due to the lots of files I moved and then changed. I'm not sure how to make the review simpler 😕~~

I've moved config.go and config_test.go back to the agent package. It's now much simpler to review!

pkg/agent/config_test.go

At first, I had moved config.go and config_test.go to a separate folder to make things a bit cleaner, but it made reviewing super hard, so I ended up keeping them in the `agent` package.

maelvls · 2024-08-26T11:18:48Z

cmd/agent.go

-		false,
-		"Enables Prometheus metrics server on the agent (port: 8081).",
-	)
+	agent.InitAgentCmdFlags(agentCmd, &agent.Flags)


Self-review: I've moved the definition of the flags for the agent command to the config package so that the struct and the flags they relate to are defined in the same file. I find it easier to keep things consistent this way.

maelvls · 2024-08-26T11:19:40Z

pkg/agent/config.go

 	UploadPath string `yaml:"upload_path,omitempty"`
 }

+type AgentCmdFlags struct {


Self-review: these were defined as package-level variables, so I moved them to be a structs so that I can test getConfiguration.

maelvls · 2024-08-26T11:20:58Z

pkg/agent/run.go

-
-// Prometheus flag enabled Prometheus metrics endpoint to run on the agent
-var Prometheus bool
+var Flags AgentCmdFlags


Self-review: This variable is public so that the cmd/agent.go file can import the global var Flags to register the command line flags. These variables were defined as package-level variables and I moved them to be a struct so that I can test getConfiguration.

maelvls · 2024-08-26T11:23:57Z

pkg/agent/config.go

+// getConfiguration combines the input configuration with the flags passed to
+// the agent and returns the final configuration as well as the Venafi client to
+// be used to upload data.
+func getConfiguration(log *log.Logger, cfg Config, flags AgentCmdFlags) (Config, client.Client, error) {


Self-review: I moved getConfiguration over from run.go as-is without any modification. This is because I think this belongs to config.go, not run.go. But let me know if it makes things too difficult to review.

maelvls · 2024-08-26T11:27:09Z

pkg/agent/config_test.go

+func TestGetConfiguration(t *testing.T) {
+	t.Run("minimal successful configuration", func(t *testing.T) {
+		got, cl, err := getConfiguration(discardLogs(t),
+			Config{Server: "http://localhost:8080", Period: 1 * time.Hour},


Self-review: This URL isn't actually used, I guess I should have picked a clearer URL like https://fake.

I've renamed it to http://api.venafi.eu.

The URL http://localhost:8080 suggested that this test was using a fake server, and didn't indicate clearly which API (the old jetstack-secure API or the newer Venafi Cloud Plaform API).

maelvls · 2024-08-26T14:00:37Z

pkg/agent/config_test.go

 	"testing"
 	"time"

+	"github.com/d4l3k/messagediff"


Self-review: I'd be in favor of removing this dependency. Even if it's just a testing dep, it isn't needed: we can use testify's Equal which already gives us a good diff.

I've removed this dependency in 21e5229.

Testify, which we already import and that I trust, has the same functionality. github.com/d4l3k/messagediff is a relatively small Go project (250 stars) that hasn't been updated since Nov 11, 2020.

inteon · 2024-08-26T15:07:25Z

go run . agent --help gives me the same result as before this change.

inteon

LGTM

maelvls · 2024-08-26T16:18:13Z

Something I forgot to mention: this PR is stacked on top of #564, meaning that:

Before merging this one, I need to make sure that the other PR that I'm stacking this on (the "base" PR) is merged.
Once the base PR is merged, I can change the base of this one to master and click "Merge".

maelvls · 2024-08-26T18:17:48Z

pkg/agent/run.go

+		logs.Log.Fatalf("Failed to read config file: %s", err)
+	}
+
+	cfg, err := ParseConfig(b, Flags.StrictMode)


Post-merge comment: Oops, it looks like I made a mistake here... It should be Flags.VenafiCloudMode, not Flags.StrictMode!

Suggested change

cfg, err := ParseConfig(b, Flags.StrictMode)

cfg, err := ParseConfig(b, Flags.VenafiCloudMode)

maelvls force-pushed the unit-test-config-code branch from 804abfd to 23d22aa Compare August 23, 2024 21:06

maelvls requested a review from wallrj August 23, 2024 21:06

maelvls force-pushed the unit-test-config-code branch from 23d22aa to 0f8b639 Compare August 26, 2024 09:04

maelvls changed the base branch from master to disable-dump-config-startup August 26, 2024 09:05

maelvls commented Aug 26, 2024

View reviewed changes

pkg/agent/config_test.go Show resolved Hide resolved

maelvls force-pushed the unit-test-config-code branch from 0f8b639 to bd8355e Compare August 26, 2024 11:15

tests: add unit tests to the code that loads config/flags

7f6334f

At first, I had moved config.go and config_test.go to a separate folder to make things a bit cleaner, but it made reviewing super hard, so I ended up keeping them in the `agent` package.

maelvls force-pushed the unit-test-config-code branch from bd8355e to 7f6334f Compare August 26, 2024 11:17

maelvls commented Aug 26, 2024

View reviewed changes

tests: use a more expressive fake URL http://api.venafi.eu

0e8875d

The URL http://localhost:8080 suggested that this test was using a fake server, and didn't indicate clearly which API (the old jetstack-secure API or the newer Venafi Cloud Plaform API).

maelvls force-pushed the unit-test-config-code branch from c8a94a5 to 0e8875d Compare August 26, 2024 13:59

maelvls commented Aug 26, 2024

View reviewed changes

tests: remove dependency on github.com/d4l3k/messagediff

21e5229

Testify, which we already import and that I trust, has the same functionality. github.com/d4l3k/messagediff is a relatively small Go project (250 stars) that hasn't been updated since Nov 11, 2020.

inteon approved these changes Aug 26, 2024

View reviewed changes

inteon merged commit f1b0e08 into disable-dump-config-startup Aug 26, 2024

inteon mentioned this pull request Aug 26, 2024

VC-33564: Disable the dump of the config file on startup #564

Merged

maelvls commented Aug 26, 2024

View reviewed changes

wallrj deleted the unit-test-config-code branch August 27, 2024 08:34

	cfg, err := ParseConfig(b, Flags.StrictMode)
	cfg, err := ParseConfig(b, Flags.VenafiCloudMode)

Conversation

maelvls commented Aug 23, 2024 • edited by inteon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maelvls commented Aug 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

maelvls Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

maelvls Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

maelvls Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maelvls Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

maelvls Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

maelvls Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maelvls Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

maelvls Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

inteon commented Aug 26, 2024

Uh oh!

inteon left a comment

Choose a reason for hiding this comment

Uh oh!

maelvls commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maelvls Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

maelvls commented Aug 23, 2024 •

edited by inteon

Loading

maelvls commented Aug 23, 2024 •

edited

Loading

maelvls Aug 26, 2024 •

edited

Loading

maelvls Aug 26, 2024 •

edited

Loading

maelvls commented Aug 26, 2024 •

edited

Loading

maelvls Aug 26, 2024 •

edited

Loading