remove mandate of GCR creds for kaniko build#1728
Closed
venkatk-25 wants to merge 6 commits intoGoogleContainerTools:masterfrom
Closed
remove mandate of GCR creds for kaniko build#1728venkatk-25 wants to merge 6 commits intoGoogleContainerTools:masterfrom
venkatk-25 wants to merge 6 commits intoGoogleContainerTools:masterfrom
Conversation
venkatk-25
requested review from
balopat,
dgageot,
nkubala,
priyawadhwa and
r2d4
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1728 +/- ##
==========================================
+ Coverage 46.4% 47.43% +1.02%
==========================================
Files 132 136 +4
Lines 6322 6670 +348
==========================================
+ Hits 2934 3164 +230
- Misses 3087 3187 +100
- Partials 301 319 +18
Continue to review full report at Codecov.
|
balopat
suggested changes
Mar 7, 2019
Contributor
balopat
left a comment
balopat
left a comment
There was a problem hiding this comment.
This looks good - but we need to add a logic for migration between the old version and the new version as you are moving fields. You can either keep the fields around as they are and just include your optionality logic or you have to implement skaffold fix logic for autoupgrading.
Contributor
Author
|
I have already made changes for skaffold fix in pkg/skaffold/schema/v1beta5/upgrade.go |
Contributor
Oh, yeah, I missed that. It looks good :) |
Contributor
Author
|
I have made changes required and moves the fix to v1beta6 |
balopat
approved these changes
Mar 11, 2019
Contributor
|
@venkatk-25 Could you do one more rebase, please? 🥇 |
…fold into make_gcr_creds_optional_in_kaniko
Contributor
Author
|
@dgageot done |
Contributor
|
There is an integration test issue: will result in (after this fix): |
balopat
suggested changes
Mar 15, 2019
| }, | ||
| } | ||
|
|
||
| if cfg.GoogleCloudConfig == nil { |
Contributor
There was a problem hiding this comment.
Suggested change
| if cfg.GoogleCloudConfig == nil { | |
| if cfg.GoogleCloudConfig != nil { |
Contributor
There was a problem hiding this comment.
and we should cover this in a unit test too if possible
| return addGoogleCloudConfig(pod, cfg.GoogleCloudConfig) | ||
| } | ||
|
|
||
| if cfg.DockerConfig == nil { |
Contributor
There was a problem hiding this comment.
Suggested change
| if cfg.DockerConfig == nil { | |
| if cfg.DockerConfig != nil { |
and we should cover this in a unit test too if possible
Contributor
|
@venkatk-25 , in another PR #1700, @azaiter is proposing another way to specify ECR secret. |
Contributor
Author
Contributor
|
@venkatk-25 if this is not the likely direction you are going to head (after #1892) , please close this PR. |
Contributor
Author
|
sure, closing this. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently Kaniko build requires configuring GCR credentials even if I am not using it,
This change makes GCR creds optional and also makes adding other registry like ECR easy in future