Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PullSecretMountPath to ClusterDetails #2975

Merged
merged 4 commits into from
Oct 3, 2019

Conversation

priyawadhwa
Copy link
Contributor

Relates to in case of new feature, this should point to issue/(s) which describes the feature

Fixes #731

Description

This will give users the option to specify where the pull secret should
be mounted within the container. This should fix #731 and enable ECR
support.

User facing changes

n/a

Next PRs.

n/a

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes unit tests
  • Mentions any output changes.
  • Adds documentation as needed: user docs, YAML reference, CLI reference.
  • Adds integration tests if needed.

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

Reviewer Notes

  • The code flow looks good.
  • Unit test added.
  • User facing changes look good.

Priya Wadhwa added 3 commits October 2, 2019 13:23
This will give users the option to specify where the pull secret should
be mounted within the container. This should fix GoogleContainerTools#731 and enable ECR
support.
@codecov
Copy link

codecov bot commented Oct 2, 2019

Codecov Report

Merging #2975 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted Files Coverage Δ
pkg/skaffold/schema/latest/config.go 100% <ø> (ø) ⬆️
pkg/skaffold/schema/defaults/defaults.go 91.08% <100%> (+0.05%) ⬆️
pkg/skaffold/build/cluster/sources/sources.go 90.9% <100%> (ø) ⬆️

pkg/skaffold/schema/defaults/defaults.go Show resolved Hide resolved
@@ -111,6 +111,27 @@ func TestSetDefaultsOnCluster(t *testing.T) {

t.CheckNoError(err)
t.CheckDeepEqual(constants.DefaultKanikoSecretName, cfg.Build.Cluster.PullSecretName)
t.CheckDeepEqual(constants.DefaultKanikoSecretMountPath, cfg.Build.Cluster.PullSecretMountPath)
Copy link
Member

Choose a reason for hiding this comment

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

Add a test for when the secret is optional and make sure PullSecretMountPath is not null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By "when the secret is optional" do you mean when PullSecret and PullSecretName are not set?

Copy link
Member

Choose a reason for hiding this comment

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

yes i think that what @prary accomplished in #2910.

Copy link
Member

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

spoke in person with @priyawadhwa and we decided this is fine for now.
We need to do some further follow up on secret management and documentation for different registries.

Thanks
Tejal.

@priyawadhwa priyawadhwa merged commit f7a385e into GoogleContainerTools:master Oct 3, 2019
@priyawadhwa priyawadhwa deleted the hostpath branch October 3, 2019 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skaffold/Kaniko with ECR
3 participants