-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 dep + remove etcd to/from cluster api docker image #9874
Add dep + remove etcd to/from cluster api docker image #9874
Conversation
Welcome @alvaroaleman! It looks like this is your first PR to kubernetes/test-infra 🎉🎉 |
preset-service-account: "true" | ||
spec: | ||
containers: | ||
- image: gcr.io/k8s-testimages/cluster-api:v20181020-5ce477da2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roberthbailey Wo builds this image? The tag I used is only correct if it will be built today at commit 5ce477d.
Does it work like this or do I have to do two prs, one for the image change, one for the presubmit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BenTheElder can confirm but I think you can do it in one commit as long as the new image has already been pushed with the relevant changes inside of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, you should update the image in the other tests so that we use the same image everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BenTheElder can confirm but I think you can do it in one commit as long as the new image has already been pushed with the relevant changes inside of it
Yeah but who does push it? I see no job on the pull that does it.
I'll update the tags for the other images when I know what it should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might have permissions to push it, but @BenTheElder certainly will.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for the delay, my inbox is RIP..
I have no idea who maintains this image.. looks like krousey?
test-infra/images/cluster-api/Dockerfile
Line 17 in be2f242
LABEL maintainer="[email protected]" |
These are not auto pushed. I can push one though, and I'm pretty sure you have permissions @roberthbailey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roberthbailey Could you please push an image with my changes and tell me the tag, so I can update the PR and we can merge it?
13d4de5
to
edff400
Compare
edff400
to
7e2ec25
Compare
preset-service-account: "true" | ||
spec: | ||
containers: | ||
- image: gcr.io/k8s-testimages/cluster-api:v20181020-5ce477da2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BenTheElder can confirm but I think you can do it in one commit as long as the new image has already been pushed with the relevant changes inside of it.
preset-service-account: "true" | ||
spec: | ||
containers: | ||
- image: gcr.io/k8s-testimages/cluster-api:v20181020-5ce477da2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, you should update the image in the other tests so that we use the same image everywhere.
images/cluster-api/Dockerfile
Outdated
@@ -23,9 +23,16 @@ ENV IMAGE=${IMAGE_ARG} | |||
ENV ETCD_VER=v3.3.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need to download & install etcd any longer. It would be useful to see if we can take it out and still have our tests pass. No need to do it as part of this PR, but could you file an issue to follow up on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened kubernetes-sigs/cluster-api#555 for that
7e2ec25
to
b245a72
Compare
@roberthbailey @BenTheElder is this waiting for another review? |
this one is a bit akward because it has both an image change and depending on that image at the same time, one of us will need to check out at the image change and push it. @alvaroaleman do you think you could split the PR out? at some point we'll probably automate the image pushes but even manually it's a little easier to just sync after the first PR and push the image |
b245a72
to
2936aa9
Compare
2936aa9
to
f6a93b0
Compare
It looks like the PR is now just changing the dockerfile, which looks good to me. /lgtm |
LGTM label has been added. Git tree hash: 7d7094569b181963a60c1004b6caae7c8ffd64a6
|
@BenTheElder Yes, I removed the commit that adds the presubmit so the PR now only changes the image. I will create a follow-up that adds the new presubmit and bumps the image in all existing presubmits. @roberthbailey Can you approve, please? |
/approve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, BenTheElder, roberthbailey The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
sorry caught back up to this, will look at the image today. |
v20190114-652973a54 should be pushed after a |
/assign @roberthbailey