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 support for ECR pod specs for kaniko #1700

Closed
wants to merge 5 commits into from

Conversation

azaiter
Copy link

@azaiter azaiter commented Feb 27, 2019

This PR adds support for proper ECR pod specs for kaniko. Previously, Kaniko pod template didn't have proper environment variables and volume mounts to have it working properly.

Kinda related issue: #731

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@azaiter
Copy link
Author

azaiter commented Feb 27, 2019

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@codecov-io
Copy link

Codecov Report

Merging #1700 into master will decrease coverage by 0.02%.
The diff coverage is 20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1700      +/-   ##
==========================================
- Coverage   46.55%   46.53%   -0.03%     
==========================================
  Files         125      125              
  Lines        5664     5669       +5     
==========================================
+ Hits         2637     2638       +1     
- Misses       2754     2758       +4     
  Partials      273      273
Impacted Files Coverage Δ
pkg/skaffold/runner/timings.go 15% <0%> (-0.39%) ⬇️
pkg/skaffold/schema/defaults/defaults.go 38.15% <25%> (-0.36%) ⬇️

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 b64a0ed...edf99a2. Read the comment docs.

@tejal29 tejal29 added the kokoro:run runs the kokoro jobs on a PR label Mar 20, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Mar 20, 2019
@tejal29
Copy link
Member

tejal29 commented Mar 20, 2019

Thank You @azaiter for this PR.
Before we move forward, there is another PR floating around, where we move PullSecret (by default GAC) to something like this:

  kaniko:
    buildContext: 
      googleCloudConfig:
        secretName: e2esecret

Maybe we should support a configurable way to specify

  1. What Container Registry this is
  2. And then secret name.
    Instead of having isECR, isSomeOtherRegistry and have branches.
    Would you be open to collaborate with @venkatk-25 to come up with a design proposal for this?

@venkatk-25
Copy link
Contributor

Created a new proposal https://docs.pivotal.io/pivotalcf/2-0/pcf-release-notes/runtime-rn.html#2.0.22 . Please give suggestions/comments

@tejal29
Copy link
Member

tejal29 commented Apr 1, 2019

Created a new proposal https://docs.pivotal.io/pivotalcf/2-0/pcf-release-notes/runtime-rn.html#2.0.22 . Please give suggestions/comments

@venkatk-25 i think the link "https://docs.pivotal.io/pivotalcf/2-0/pcf-release-notes/runtime-rn.html#2.0.22 is incorrect, you meant to post #1892 ?
Also, can we capture all of this in an actual document? Its easy for commenting VS a github issue and will remain in the git history.

@dgageot
Copy link
Contributor

dgageot commented Jul 2, 2019

Hi @azaiter this PR seems to be stalled for quite some time. Would you like to update it?

@dgageot
Copy link
Contributor

dgageot commented Jul 5, 2019

I'm going to close this PR because of the lack of activity. I'm sorry we couldn't get this merged in. Please feel free to reopen, we'll be more that happy to help get this done!

@dgageot dgageot closed this Jul 5, 2019
@azaiter
Copy link
Author

azaiter commented Jul 8, 2019

Sorry, I've been super busy last week. Since @venkatk-25 raised a design proposal based on @tejal29 's suggestion to generalize the approach towards setting env vars and mounting files to Kaniko pod, in addition to #1906 PR that actually does that, I think this PR is no longer the optimal approach unless if you see something that is worth building upon from this PR, in which I'm also more happy to help (time permitting). Thanks!

@azaiter azaiter mentioned this pull request Oct 4, 2019
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.

None yet

7 participants