Added support for multiple --from-env flags #101646

lauchokyip · 2021-04-30T04:16:56Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Added support for multiple --from-env flags

Which issue(s) this PR fixes:

Fixes kubernetes/kubectl#1033

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Adding support for multiple --from-env-file flags

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2021-04-30T04:17:03Z

@lauchokyip: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lauchokyip · 2021-04-30T04:17:55Z

/cc @KnVerey

KnVerey · 2021-05-11T19:32:52Z

staging/src/k8s.io/kubectl/pkg/cmd/create/create_configmap_test.go

@@ -429,20 +476,28 @@ func TestCreateConfigMap(t *testing.T) {
 	}
 }

-func setupEnvFile(lines ...string) func(*testing.T, *ConfigMapOptions) func() {
+func setupEnvFile(filenames []string, lines [][]string) func(*testing.T, *ConfigMapOptions) func() {


Do the tests actually need to control the filenames? If not, I'd suggest simply including the index in the filename below, to simplify setup. If they do, I'd suggest making the argument filemap map[string][]string (e.g. "filename": {"l1", "l2"} so that it's easier to understand what's going where.

no, it's no needed 😕

KnVerey

Great work! I have a few small questions/quibbles, but I tried it out locally and it is working nicely.

KnVerey · 2021-05-11T21:47:21Z

staging/src/k8s.io/kubectl/pkg/cmd/create/create_configmap.go

@@ -87,7 +87,7 @@ type ConfigMapOptions struct {
 	// LiteralSources to derive the configMap from (optional)
 	LiteralSources []string
 	// EnvFileSource to derive the configMap from (optional)
-	EnvFileSource string
+	EnvFileSource []string


Let's change the variable/method naming to be plural all around, like it is for the other sources.

Suggested change

EnvFileSource []string

EnvFileSources []string

KnVerey · 2021-05-11T21:59:07Z

staging/src/k8s.io/kubectl/pkg/cmd/create/create_configmap.go

-			return fmt.Errorf("error reading %s: %v", envFileSource, err.Err)
-		default:
-			return fmt.Errorf("error reading %s: %v", envFileSource, err)
+func handleConfigMapFromEnvFileSource(configMap *corev1.ConfigMap, envFileSources []string) error {


Suggested change

func handleConfigMapFromEnvFileSource(configMap *corev1.ConfigMap, envFileSources []string) error {

func handleConfigMapFromEnvFileSources(configMap *corev1.ConfigMap, envFileSources []string) error {

KnVerey · 2021-05-11T22:07:52Z

staging/src/k8s.io/kubectl/pkg/cmd/create/create_configmap_test.go

@@ -414,8 +458,11 @@ func TestCreateConfigMap(t *testing.T) {
 					defer teardown()
 				}
 			}
+			err := configMapOptions.Validate()


Why is this added?

I was the one that remove the generator of the ConfigMap (https://github.com/pulls?q=is%3Apr+author%3Alauchokyip+archived%3Afalse+is%3Aclosed) and when I wrote the test cases for example this test cases

kubernetes/staging/src/k8s.io/kubectl/pkg/cmd/create/create_configmap_test.go

Line 386 in a45aeb6

"create_invalid_configmap_too_many_args": {

I forgot that Validate() would actually catch the error here

kubernetes/staging/src/k8s.io/kubectl/pkg/cmd/create/create_configmap.go

Line 208 in a45aeb6

if len(o.EnvFileSource) > 0 && (len(o.FileSources) > 0 || len(o.LiteralSources) > 0) {

so I think we have to add that now. Same as what I did for create_secret_test.go

kubernetes/staging/src/k8s.io/kubectl/pkg/cmd/create/create_secret_test.go

Line 480 in a45aeb6

err := secretOptions.Validate()

What I still don't understand is why this PR makes this change necessary. If the errors the cases you point to are meant to hit are in Validate and only createConfigMap is being called on master today, then how are the tests passing on master? Are they hitting errors other than the ones they're meant to? Incidentally, that possibility is one reason why I personally prefer to make error expectations strings to match on rather than booleans, to provide better assurances that the error that happens is the one I expect.

To be clear, I'm not opposed to this change--just want to make sure I understand it.

Will take a look into it

Turn out the error happened during createConfigMap
For instance the test case create_invalid_configmap_too_many_args, instead of throwing the error (from-env-file cannot be combined with from-file or from-literal) it actually tries to create a config map from the file and it cannot read the file therefore throws an error

I agree comparing string error is way better so I will make a change from that

Thanks for digging into this, and great job with the improvements to this file!

@lauchokyip your latest rebase seems to have lost the changes from this discussion, i.e. the ones visible in https://github.com/kubernetes/kubernetes/compare/cc37d5df54caaaa50291d37c93042d73ea506778..977ca6feb4fe433e667df051dfa4b4c21e4fce02. Can you please restore them? Note that one of the error strings that shows up in that diff answers @eddiezane question. 😄

Yes I am restoring them! Thanks for the reminder 😃

lauchokyip · 2021-05-19T20:50:26Z

/retest

lauchokyip · 2021-05-27T17:30:58Z

staging/src/k8s.io/kubectl/pkg/cmd/create/create_configmap_test.go

 			if !test.expectErr && err != nil {
-				t.Errorf("test %s, unexpected error: %v", name, err)
+				t.Errorf("unexpected error: %v", err)


removed the name because if the unit test fails it would already output the name so in my opinion it's repetitive and confusing

lauchokyip · 2021-05-27T17:33:00Z

/assign @eddiezane

eddiezane · 2021-05-27T17:51:20Z

staging/src/k8s.io/kubectl/pkg/cmd/create/create_configmap.go

@@ -86,8 +86,8 @@ type ConfigMapOptions struct {
 	FileSources []string
 	// LiteralSources to derive the configMap from (optional)
 	LiteralSources []string
-	// EnvFileSource to derive the configMap from (optional)
-	EnvFileSource string


This struct and field are exported. This will be a breaking change to any external consumers. Not sure if there are any 🤔.

https://cs.k8s.io/?q=EnvFileSource&i=nope&files=&excludeFiles=&repos=

I guess with our refactoring of the commands anyways it doesn't matter.

My instinct is that the flags used to build a CLI are particular to that CLI itself, and not intended for library use. That search isn't really showing any consumers in any case; Kustomize's implementation matches but is independent, and driver-registrar is just showing up as vendored, not used. The rest is this code itself and its mirror. So IMO there's no problem with this change, but we can check with other approvers if you're worried about it. :)

KnVerey · 2021-05-27T18:12:02Z

staging/src/k8s.io/kubectl/pkg/cmd/create/create_configmap_test.go

-		expectErr bool
+		expected     *corev1.ConfigMap
+		expectErr    bool
+		expectErrStr string


I'd suggest consolidating into only the string field. We should never expect a blank error, so the boolean is kinda redundant and needs to be synchronized (e.g. right now you could set an error string but forget to set expectErr to true and it wouldn't get checked).

makes sense

KnVerey

/lgtm

KnVerey · 2021-05-28T15:47:19Z

staging/src/k8s.io/kubectl/pkg/cmd/create/create_configmap.go

@@ -86,8 +86,8 @@ type ConfigMapOptions struct {
 	FileSources []string
 	// LiteralSources to derive the configMap from (optional)
 	LiteralSources []string
-	// EnvFileSource to derive the configMap from (optional)
-	EnvFileSource string


My instinct is that the flags used to build a CLI are particular to that CLI itself, and not intended for library use. That search isn't really showing any consumers in any case; Kustomize's implementation matches but is independent, and driver-registrar is just showing up as vendored, not used. The rest is this code itself and its mirror. So IMO there's no problem with this change, but we can check with other approvers if you're worried about it. :)

KnVerey · 2021-05-28T15:48:36Z

staging/src/k8s.io/kubectl/pkg/cmd/create/create_configmap_test.go

@@ -414,8 +458,11 @@ func TestCreateConfigMap(t *testing.T) {
 					defer teardown()
 				}
 			}
+			err := configMapOptions.Validate()


Thanks for digging into this, and great job with the improvements to this file!

eddiezane · 2021-07-14T13:42:51Z

@lauchokyip sorry I missed this. Please rebase when you get a chance and we can get merged after code thaw.

lauchokyip · 2021-07-18T15:16:35Z

@eddiezane Rebased. PTAL 😄

eddiezane · 2021-07-19T17:37:26Z

Thanks!

What's the intended behavior for duplicate entries? Second one overrides the first?

KnVerey · 2021-07-19T19:07:06Z

What's the intended behavior for duplicate entries? Second one overrides the first?

Second one is rejected. Regardless of original source, the entries are added with addKeyFromLiteralToConfigMap, which as documented "return[s] an error if the key is not valid or if the key already exists." This behavior didn't need to change from the original single-file implementation, since it was already a possibility there.

KnVerey · 2021-07-21T01:45:18Z

In case my comment is hard to find, I noticed yesterday that the latest force push seems to have lost some of the previous requested changes: #101646 (comment)

lauchokyip · 2021-07-25T20:09:20Z

@KnVerey and @eddiezane , I restored the previous changes, PTAL, thank you 👍🏼

finnickkim · 2021-07-25T21:42:11Z

[ ]

KnVerey

Just a tiny request to revert an additional change that slipped in, otherwise looks good!

KnVerey · 2021-07-27T19:29:58Z

staging/src/k8s.io/kubectl/pkg/cmd/create/create_configmap.go

@@ -68,8 +68,8 @@ var (
 		  # Create a new config map named my-config from the key=value pairs in the file
 		  kubectl create configmap my-config --from-file=path/to/bar

-		  # Create a new config map named my-config from an env file
-		  kubectl create configmap my-config --from-env-file=path/to/bar.env`))
+		  # Create a new configmap named my-config from an env file


Please revert this change--it's spelled as two words in all the other help text above

Will do! Thanks for the fast review!

lauchokyip · 2021-07-28T02:02:18Z

/retest

lauchokyip · 2021-07-28T02:02:36Z

/retest-required

KnVerey · 2021-07-28T15:14:18Z

/lgtm

eddiezane · 2021-07-28T15:19:59Z

Great job team! Awesome work on this one.

/approve

k8s-ci-robot · 2021-07-28T15:20:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eddiezane, lauchokyip

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:

~~staging/src/k8s.io/kubectl/OWNERS~~ [eddiezane]

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

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 30, 2021

k8s-ci-robot requested review from dougsland and rikatz April 30, 2021 04:17

k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 30, 2021

k8s-ci-robot requested a review from KnVerey April 30, 2021 04:17

KnVerey reviewed May 11, 2021

View reviewed changes

lauchokyip force-pushed the fixConfigMap branch 2 times, most recently from e92e84b to e00ee11 Compare May 19, 2021 14:42

lauchokyip force-pushed the fixConfigMap branch from e00ee11 to cc37d5d Compare May 27, 2021 17:26

lauchokyip commented May 27, 2021

View reviewed changes

k8s-ci-robot assigned eddiezane May 27, 2021

eddiezane reviewed May 27, 2021

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 27, 2021

KnVerey reviewed May 27, 2021

View reviewed changes

lauchokyip force-pushed the fixConfigMap branch from cc37d5d to 977ca6f Compare May 27, 2021 18:27

KnVerey reviewed May 28, 2021

View reviewed changes

k8s-ci-robot assigned KnVerey May 28, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2021

lauchokyip force-pushed the fixConfigMap branch from 977ca6f to 45898b2 Compare July 18, 2021 15:15

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 18, 2021

lauchokyip requested a review from KnVerey July 18, 2021 15:16

lauchokyip force-pushed the fixConfigMap branch 2 times, most recently from 15dbb42 to 8b91b8c Compare July 25, 2021 19:37

KnVerey reviewed Jul 27, 2021

View reviewed changes

Added support for multiple --from-env flags

99bc2b0

lauchokyip force-pushed the fixConfigMap branch from 8b91b8c to 99bc2b0 Compare July 28, 2021 00:50

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2021

k8s-ci-robot merged commit 0d99701 into kubernetes:master Aug 5, 2021

SIG CLI automation moved this from Needs Approver to Done Aug 5, 2021

k8s-ci-robot added this to the v1.23 milestone Aug 5, 2021

lauchokyip mentioned this pull request Aug 17, 2021

Added support for multiple --from-env flags #104232

Merged

lauchokyip deleted the fixConfigMap branch August 17, 2021 23:19

lauchokyip mentioned this pull request Nov 1, 2021

Do not raise an error proposing to use '--overwrite' when labeling with the same value #105936

Merged

	func handleConfigMapFromEnvFileSource(configMap *corev1.ConfigMap, envFileSources []string) error {
	func handleConfigMapFromEnvFileSources(configMap *corev1.ConfigMap, envFileSources []string) error {

Added support for multiple --from-env flags #101646

Added support for multiple --from-env flags #101646

Conversation

lauchokyip commented Apr 30, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Apr 30, 2021

lauchokyip commented Apr 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KnVerey 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

lauchokyip May 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauchokyip May 27, 2021 • edited

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

lauchokyip commented May 19, 2021

Choose a reason for hiding this comment

lauchokyip commented May 27, 2021

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

KnVerey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddiezane commented Jul 14, 2021

lauchokyip commented Jul 18, 2021

eddiezane commented Jul 19, 2021

KnVerey commented Jul 19, 2021

KnVerey commented Jul 21, 2021

lauchokyip commented Jul 25, 2021 • edited

finnickkim commented Jul 25, 2021

KnVerey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauchokyip commented Jul 28, 2021

lauchokyip commented Jul 28, 2021

KnVerey commented Jul 28, 2021

eddiezane commented Jul 28, 2021

k8s-ci-robot commented Jul 28, 2021

lauchokyip commented Apr 30, 2021 •

edited

lauchokyip May 16, 2021 •

edited

lauchokyip May 27, 2021 •

edited

lauchokyip commented Jul 25, 2021 •

edited