Kaniko secret

[prary]

kaniko pull secret should be optional
Resolving issue #2190

[balopat]

Actually, this is not how we should implement the optionality.
The logic should be implemented in secret.go, here -- when both PullSecretName and PullSecret are empty then that should be fine.

[prary]

This is how it should be right?

func setupPullSecret() {
	if b.PullSecret == "" {
		return func() {}, nil
        }
        rest of the secret creation code
}

[codecov]

Codecov Report

Merging #2321 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2321      +/-   ##
==========================================
- Coverage   61.86%   61.84%   -0.02%     
==========================================
  Files         186      186              
  Lines        8050     8052       +2     
==========================================
  Hits         4980     4980              
- Misses       2679     2681       +2     
  Partials      391      391

Impacted Files	Coverage Δ
pkg/skaffold/build/cluster/cluster.go	18.91% <ø> (ø)	⬆️
pkg/skaffold/build/cluster/secret.go	0% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6bd1d50...9dc22c8. Read the comment docs.

[dgageot]

@prary could you add a unit test for the new code?

[prary]

@prary could you add a unit test for the new code?

Sure I will add

[tejal29]

@prary, Here is an example of creating a mock client and adding secrets

skaffold/pkg/skaffold/deploy/status_check_test.go

Line 180 in 6ed64d6

objs := make([]runtime.Object, len(test.deps))

// Create Secrets	
		objs := make([]runtime.Object, len(test.secrets))
			for i, s := range test.secrets {
				objs[i] = dep
			}
// Create a fake client with Secrets added
			client := fakekubeclientset.NewSimpleClientset(objs...)
// Pass the fake client to the method using it

actual, err := getDeployments(client, "test", labeller, time.Duration(200)*time.Second)

[tejal29]

In your case, you need to mock the function call to kubernetes.GetClientset() to actually return the fakeclient.

An example is here:

https://github.com/GoogleContainerTools/skaffold/blob/master/pkg/skaffold/kubernetes/portforward/resource_forwarder.go#L43

And In tests, you mock by :

https://github.com/GoogleContainerTools/skaffold/blob/master/pkg/skaffold/kubernetes/portforward/resource_forwarder_test.go#L347

[tejal29]

Unfortunately you ended up changing a file which does not have any test coverage. :(

In this case, the best way to test would be

1. create a new test file secret_test.go
2. create a testRunContext with ClusterDetails where b.PullSecret and b.PullSecretName is empty. Create a testBuilder

runCtx := &runcontext.RunContext{Cgf: latest.Pipeline{}}
testBuilder := NewBuilder(runCtx)
expectedFunc, expectedErr := testBuilder.setupPullSecret(ioutil.Discard)

See an example:

skaffold/pkg/skaffold/test/test_test.go

Line 50 in 10e3a55

runCtx := &runcontext.RunContext{

you can add tests for

1. when both are empty
2. When b.PullSecret is empty

For test case 2, you will have to use instructions mentioned here #2321 (comment)

[tejal29]

I think @dgageot covered this #2321