Added return because without it if the test fails it will throw a panic for the next line when we want to dereference the err err.Error() but the err will be nil. Logically, it makes sense to have return there because we want to move on to the next test cases if the current fail.

I agree that only one of the four conditionals here is intended to apply to any given case. The same would be true for the configmap test changes too, right?

FWIW I like using testify's require for this, e.g.:

if test.expectErr != "" {
  require.EqualError(t, test.expectErr, err.Error())
} 
require.NoError(t, err)
if !apiequality.Semantic.DeepEqual ... {
}

It looks like testify is already used in other create tests, so that could be an option, if you want (up to you).

Thanks, didn't know that exists 👍🏼

Actually I think I didn't explain it well enough,
For example if I have a test case

		"foo": {
			secretName:  "foo",
			fromLiteral:    []string{"key1=value1"},
			expectErr:   `foo`,
		},

when it will first reach this line, which is fine because we are expecting an error and the err is nil

if test.expectErr != "" && err == nil

But when it reaches the next line, err.Error() will cause a panic because err is nil.

if test.expectErr != "" && test.expectErr != err.Error() {

If I do,

if test.expectErr != "" {
  require.EqualError(t, test.expectErr, err.Error())
} 
require.NoError(t, err)
if !apiequality.Semantic.DeepEqual ... {
}

woudln't require.EqualError(t, test.expectErr, err.Error()) cause panic too if err is nil?

Ah yes, you're right. You'd need an additional require.Error(t, err) as the second line.

If that's the case, do you think require is better or return? Personally, I think return would make the code cleaner.

I find the require version more succinct/readable, but it's a personal preference. I'll LGTM either way once the error wording fix is pushed.

sure! let's go with require then, I will play around with it and make an update. Thank you

I agree that only one of the four conditionals here is intended to apply to any given case. The same would be true for the configmap test changes too, right?

FWIW I like using testify's require for this, e.g.:

if test.expectErr != "" {
  require.EqualError(t, test.expectErr, err.Error())
} 
require.NoError(t, err)
if !apiequality.Semantic.DeepEqual ... {
}

It looks like testify is already used in other create tests, so that could be an option, if you want (up to you).

This error message changed slightly. Was it intentional or ported over from the configmap code?

Thanks for the quick review! I ported the code from configmap and made the careless mistake 😅 , will make changes now

If you want you can simplify this one slightly with require as well, and have it return instead of attempt to run the DeepEqual if the assertion fails. My full suggestion is below but this is also fine as-is.

if test.expectErr == "" {
  require.NoError(err)
  if !apiequality.Semantic.DeepEqual(secret, test.expected) {
    t.Errorf("\nexpected:\n%#v\ngot:\n%#v", test.expected, secret)
  }
} else {
  require.Error(t, err)
  require.EqualError(t, err, test.expectErr)
}

Let's simplify it then, I think this change is better ! will make a change now

What do you do if you have duplicate keys? Is this properly handled by this function? Specifically if they are coming from different files with the same name in different directories?

@eddiezane actually asked the same questions 😄 and @KnVerey answered it over here (#101646 (comment))

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.

nit: Add a line here please. Convention is:

built in

3rd party

k8s