Simplify flag and configfile parsing & add tests #6244

inteon · 2023-07-31T14:31:18Z

Pull Request Motivation

The current approach requires a lot of workarounds. This PR introduces an alternative shorter workaround.
For context: the problem we want to fix is that in order to load the configfile, we have to parse the "--config" flag. Also, the flags should overwrite the configfile options.
The solution: 1. we parse all flags; 2. we load the configfile; 3. we re-parse the flags

The PR also includes some beautiful tests that validate this behavior.

Kind

/kind cleanup

Release Note

NONE

AcidLeroy · 2023-07-31T16:19:47Z

I see that you removed the tests for the flag precedence in this PR. Should we still have a unit test to verify that the behavior we want with this change is preserved?

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon · 2023-08-17T11:16:04Z

cmd/controller/app/start_test.go

@@ -0,0 +1,190 @@
+/*


This file replaces controller_test.go

inteon · 2023-08-17T16:28:05Z

I see that you removed the tests for the flag precedence in this PR. Should we still have a unit test to verify that the behavior we want with this change is preserved?

I added some tests that helped me build confidence in this approach.

AcidLeroy · 2023-08-17T17:27:41Z

Looks good to me! Great improvement!

wallrj

Sorry Tim,

I can't understand what's going on here.

You'll need to add some comments explaining how it all works.
Especially where you're supplying functions as arguments with names like fn.

I see that Cody has already approved it and I trust that he knows the context.
So I'll add a lgtm in case you just want to merge it anyway.

/lgtm
/hold

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

wallrj

Thanks Tim,

Thanks for the tests. I didn't see those before.
And thanks for the comments, explaining the "Why" in the code.

As I said during the morning standup, I think it would be more logical if the code specifically parsed the --config option first, then parsed the config file, then parsed the remaining CLI options.

OR

That the options are all parsed at once into a sparse structure, where every field is a pointer, so that we know definitively which CLI options have been supplied.

Then merge those into the field values that have been parsed from the config file, if one was supplied.

It sounds like that's not currently possible with Cobra.

So I'll lgtm this.

/lgtm
/hold incase you have a response to my question about the validation of the config file.

wallrj · 2023-08-18T11:22:40Z

cmd/controller/app/start_test.go

+		{
+			yaml: `
+apiVersion: controller.config.cert-manager.io/v1alpha1
+kind: ControllerConfiguration
+kubeConfig: "<invalid>"
+`,
+			args: func(tempFilePath string) []string {
+				return []string{"--config=" + tempFilePath, "--kubeconfig=valid"}
+			},
+			expConfig: configFromDefaults(func(tempDir string, cc *config.ControllerConfiguration) {
+				cc.KubeConfig = "valid"
+			}),
+		},


You've used the values <invalid> and valid.
Do you intend to demonstrate that an invalid value can be used in the config file,
if a valid value has been supplied on the config file?

I think that both the config file and the command line values should validated.
An invalid configfile should be rejected, regardless of the options supplied on the command line.

Invalid, valid was already used in the original test code: https://github.com/cert-manager/cert-manager/pull/6244/files/48cc19bee3ede809be80a5295c2daacc19f0071c#diff-41e02d5cc47d0e9e7b0461d73abb901804809b9de11a77596f5565656892457bL26-L40

I think the validation part is lacking anyway and should be improved in a future PR.

jetstack-bot · 2023-08-18T14:14:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon, wallrj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [inteon,wallrj]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

inteon · 2023-08-18T15:09:41Z

/unhold

inteon added this to the 1.13 milestone Jul 31, 2023

inteon force-pushed the simplify_flag_and_configfile_parsing branch from e574c85 to 3d45871 Compare July 31, 2023 20:02

jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. and removed dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels Jul 31, 2023

inteon force-pushed the simplify_flag_and_configfile_parsing branch from 3d45871 to 1142782 Compare July 31, 2023 20:03

jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Jul 31, 2023

inteon force-pushed the simplify_flag_and_configfile_parsing branch from 1142782 to 3df750a Compare August 10, 2023 14:13

jetstack-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 10, 2023

inteon force-pushed the simplify_flag_and_configfile_parsing branch from 3df750a to b0d2d09 Compare August 16, 2023 11:07

jetstack-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 16, 2023

inteon force-pushed the simplify_flag_and_configfile_parsing branch 2 times, most recently from fe3c29d to 0429a47 Compare August 17, 2023 08:00

inteon changed the title ~~Simplify flag and configfile parsing~~ Simplify flag and configfile parsing & add tests Aug 17, 2023

inteon force-pushed the simplify_flag_and_configfile_parsing branch from 0429a47 to 955db5a Compare August 17, 2023 10:02

jetstack-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 17, 2023

inteon force-pushed the simplify_flag_and_configfile_parsing branch 2 times, most recently from dfdbeb8 to b521198 Compare August 17, 2023 10:52

simplify configfile loading logic

f1b8952

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon force-pushed the simplify_flag_and_configfile_parsing branch from b521198 to f1b8952 Compare August 17, 2023 11:13

inteon commented Aug 17, 2023

View reviewed changes

cmd/controller/app/start_test.go

@@ -0,0 +1,190 @@

/*

Copy link

Member Author

inteon Aug 17, 2023

Choose a reason for hiding this comment

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

This file replaces controller_test.go

inteon requested a review from wallrj August 17, 2023 15:13

wallrj reviewed Aug 18, 2023

View reviewed changes

jetstack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 18, 2023

jetstack-bot assigned wallrj Aug 18, 2023

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 18, 2023

add comments and improve variable names

48cc19b

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

jetstack-bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 18, 2023

wallrj approved these changes Aug 18, 2023

View reviewed changes

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 18, 2023

jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 18, 2023

jetstack-bot merged commit f69cbfc into cert-manager:master Aug 18, 2023
7 checks passed

inteon deleted the simplify_flag_and_configfile_parsing branch August 18, 2023 15:10

jetstack-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Sep 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify flag and configfile parsing & add tests #6244

Simplify flag and configfile parsing & add tests #6244

inteon commented Jul 31, 2023 •

edited

AcidLeroy commented Jul 31, 2023

inteon Aug 17, 2023

inteon commented Aug 17, 2023

AcidLeroy commented Aug 17, 2023

wallrj left a comment

wallrj left a comment

wallrj Aug 18, 2023

inteon Aug 18, 2023 •

edited

jetstack-bot commented Aug 18, 2023

inteon commented Aug 18, 2023

Simplify flag and configfile parsing & add tests #6244

Simplify flag and configfile parsing & add tests #6244

Conversation

inteon commented Jul 31, 2023 • edited

Pull Request Motivation

Kind

Release Note

AcidLeroy commented Jul 31, 2023

inteon Aug 17, 2023

Choose a reason for hiding this comment

inteon commented Aug 17, 2023

AcidLeroy commented Aug 17, 2023

wallrj left a comment

Choose a reason for hiding this comment

wallrj left a comment

Choose a reason for hiding this comment

wallrj Aug 18, 2023

Choose a reason for hiding this comment

inteon Aug 18, 2023 • edited

Choose a reason for hiding this comment

jetstack-bot commented Aug 18, 2023

inteon commented Aug 18, 2023

inteon commented Jul 31, 2023 •

edited

inteon Aug 18, 2023 •

edited