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

Adding Git Buildcontext #332

Closed
wants to merge 18 commits into from
Closed

Adding Git Buildcontext #332

wants to merge 18 commits into from

Conversation

krtkvrm
Copy link
Contributor

@krtkvrm krtkvrm commented Sep 3, 2018

Fixes #269
Added Feature to use Git Repository as Build Context, using --context flag.
Private Repositories can also be used by using Personal Access Tokens.

@container-tools-bot
Copy link
Collaborator

Hi @vkartik97. Thanks for your PR.

I'm waiting for a GoogleContainerTools member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@priyawadhwa
Copy link
Collaborator

Hey @vkartik97 , thanks for contributing! This looks good, could you add an integration test for this? The steps would be:

  1. Add a test Dockerifle Dockerfile_test_git here
  2. Similarly to how we test gcs buckets, add gitRepoTests, change the context flag in BuildImage for the kaniko image and change the context for the Docker image
  3. You could probably use the kaniko repo itself to test, with a really simple Dockerfile:
FROM scratch
COPY LICENSE /LICENSE

Instructions for running the integration tests locally can be found here. Let me know if you have any questions!

@krtkvrm
Copy link
Contributor Author

krtkvrm commented Sep 12, 2018

Hi @priyawadhwa

@priyawadhwa
Copy link
Collaborator

Hey @vkartik97,

  1. You don't have to add the env variable, since kaniko is a public repo so anyone should be able to run the test locally
  2. This is on the right track, but you don't need to build the image twice. Instead, just change contextPath to equal git://<kaniko repository> if the current Dockerfile lives in gitRepoTests, and then it will build correctly. You should be able to remove the second build entirely.

@priyawadhwa
Copy link
Collaborator

Hey @vkartik97, how is this going?

Alternatively, we could also merge this PR and I could write an integration up for it afterwards.

@krtkvrm
Copy link
Contributor Author

krtkvrm commented Oct 2, 2018

@priyawadhwa
Please review the Integration Tests

@priyawadhwa
Copy link
Collaborator

@vkartik97 this is looking good! Just one more thing to add.

The integraiton tests work by building a Dockerfile with kaniko and with Docker, and then comparing the images.

You just need to change the Docker-built image to point to the same repository as well, the source context for that is here

Right now it assumes the current directory, so you'll just have to add some code to change it to point to the repo if building the git Dockerfile.

@krtkvrm
Copy link
Contributor Author

krtkvrm commented Oct 5, 2018

@priyawadhwa
Please review the changes

integration/images.go Outdated Show resolved Hide resolved
integration/images.go Outdated Show resolved Hide resolved
@krtkvrm
Copy link
Contributor Author

krtkvrm commented Nov 2, 2018

@priyawadhwa Please review

@priyawadhwa
Copy link
Collaborator

Hey @vkartik97 sorry for the late response, I think this needs another rebase.

Copy link
Collaborator

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

Thanks for your quick rebase! It's looking good, I just left a couple comments and we should be all set.

Makefile Show resolved Hide resolved
integration/images.go Outdated Show resolved Hide resolved
integration/images.go Outdated Show resolved Hide resolved
integration/images.go Outdated Show resolved Hide resolved
integration/images.go Outdated Show resolved Hide resolved
@krtkvrm
Copy link
Contributor Author

krtkvrm commented Feb 6, 2019

@priyawadhwa Please review!

@krtkvrm
Copy link
Contributor Author

krtkvrm commented Feb 21, 2019

@priyawadhwa
Can you please tell me why is kokoro build failing?

@priyawadhwa
Copy link
Collaborator

Hey @vkartik97 thanks for the ping, sorry for the delay!

I can look into the kokoro failure -- would you mind if I pushed a few changes to this branch once I figure out what's wrong?

@krtkvrm
Copy link
Contributor Author

krtkvrm commented Feb 21, 2019

would you mind if I pushed a few changes to this branch once I figure out what's wrong?

No issues, and thanks for helping me here !

@priyawadhwa
Copy link
Collaborator

Hey @vkartik97 , could you give me permission to push to this PR? Instructions here. Thanks!

@krtkvrm
Copy link
Contributor Author

krtkvrm commented Feb 22, 2019

@priyawadhwa Done

@priyawadhwa
Copy link
Collaborator

Hey @vkartik97 -- I'm not sure why but I still don't have permission to push to your branch.

Instead, I've pushed a branch here built off of yours with a couple changes to integration_test.go and images.go. I also delete Dockerfile_test_git (looks like we don't actually need it). That commit is here.

Would you be able to incorporate those changes and push them yourself?

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@krtkvrm
Copy link
Contributor Author

krtkvrm commented Apr 8, 2019

@priyawadhwa please review

@abergmeier
Copy link
Contributor

Can we get this along?

@priyawadhwa
Copy link
Collaborator

@vkartik97 are you still working on this? It looks like during rebasing some additional files may have been added (Dockerfiles that aren't related to this PR).

It may be easiest to open a new PR with just the changes in the branch I mentioned earlier, instead of trying to fix the current commit history. WDYT?

@krtkvrm
Copy link
Contributor Author

krtkvrm commented May 18, 2019

@priyawadhwa Yes, I am still working on this.
I'll open a new PR!

@krtkvrm krtkvrm closed this May 20, 2019
@krtkvrm
Copy link
Contributor Author

krtkvrm commented May 22, 2019

@priyawadhwa Opened new PR #672

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.

Create a github buildcontext
6 participants