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

Move docker image to break circular dependency #3002

Merged
merged 3 commits into from
Jul 3, 2019

Conversation

vvcephei
Copy link
Member

The docker image build was moved out of this repo in the 5.2.x branch to break a circular dependency, but the circular dependency was already present in the 5.1.x branch. This PR corrects the issue.

@vvcephei vvcephei requested a review from a team as a code owner June 21, 2019 19:25
@vvcephei
Copy link
Member Author

Just getting the PR started to see what happens with the build.

@vvcephei
Copy link
Member Author

I've moved the images to https://github.com/vvcephei/ksql-images/tree/5.1.x , but there are a couple of problems...

The big blocker is that I can't create a PR to create a new branch. I need a committer to create the 5.1.x branch on https://github.com/confluentinc/ksql-images

@rodesai , can you help out?

@vvcephei
Copy link
Member Author

Also, I'm not sure why the build failed. It failed on Twistlock being unable to find 368821881613.dkr.ecr.us-west-2.amazonaws.com/confluentinc/ksql-cli:PR-3002-3, but during the actual package phase, I see it claimed it actually did successfully tag the image.

Ideas?

@rodesai
Copy link
Contributor

rodesai commented Jun 25, 2019

The big blocker is that I can't create a PR to create a new branch. I need a committer to create the 5.1.x branch on https://github.com/confluentinc/ksql-images

@vvcephei you are already an admin for ksql-images.

Also, I'm not sure why the build failed. It failed on Twistlock being unable to find 368821881613.dkr.ecr.us-west-2.amazonaws.com/confluentinc/ksql-cli:PR-3002-3, but during the actual package phase, I see it claimed it actually did successfully tag the image.

The twistlock is something the @confluentinc/tools team merged a few days ago. Can someone from there comment on this?

@ewencp
Copy link
Contributor

ewencp commented Jun 25, 2019

Did you check if the image was still there? There's always a chance something caused it to be cleaned up during the build. Didn't check thoroughly, but pull failed for me.

Also, /cc @xiangxin72 who might have additional insight.

@xli1996
Copy link
Member

xli1996 commented Jun 25, 2019

@vvcephei Can you provide me the logs that twistlock failed? It may caused by either that the image is cleaned or wrong tags.

@vvcephei
Copy link
Member Author

@xli1996 , thanks for taking a look. Here's the logs: https://jenkins.confluent.io/job/confluentinc-pr/job/ksql/job/PR-3002/3/consoleText

Fair warning, I am not familiar with this build at all, so we may need to do some sleuthing to figure how how it's supposed to go.

@xli1996
Copy link
Member

xli1996 commented Jun 25, 2019

I don't see major difference between #2059 and this one, and the master branch works fine. The logs claims that the image is built successfully, maybe I will try disable twistlock temporary with dockerScan=false to see whether it can be uploaded to ECR, since if the twistlock scan failed, it will block images be uploaded.

@xli1996
Copy link
Member

xli1996 commented Jun 27, 2019

The twistlock issue seems to be flaky and only happen in ksql pr build, no idea what really the issue is. Maybe spend some time later to investigate, but @vvcephei the build is green now.

@spena
Copy link
Member

spena commented Jul 1, 2019

@vvcephei @rodesai is this PR ready to be merged?

5.1.x failures are blocking our Jenkins job that publishes 5.2.x/5.3.x/master artifacts to the repos. I think this PR fixes that failure.

Copy link
Member

@spena spena left a comment

Choose a reason for hiding this comment

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

@vvcephei This patch looks good. I asked @rodesai about it, and he thinks it looks good too.
Can we merge it?

@spena spena requested a review from a team July 3, 2019 17:29
@vcrfxia
Copy link
Contributor

vcrfxia commented Jul 3, 2019

Merging to fix the build.

@vcrfxia vcrfxia merged commit cd87cb8 into confluentinc:5.1.x Jul 3, 2019
@vvcephei
Copy link
Member Author

vvcephei commented Jul 9, 2019

Hey, thanks, all. Sorry, I haven't checked in since your comments. I'll follow up and make sure that the image build gets added to the ksql-images repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants