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.

Sign up for GitHub

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

Jump to bottom

Decoupling the image build process from the flow #221

Merged
merged 1 commit into from
Apr 28, 2021
Merged

Decoupling the image build process from the flow #221

merged 1 commit into from
Apr 28, 2021

Conversation

IlanZuckerman
Copy link
Collaborator

fix #211

@IlanZuckerman IlanZuckerman self-assigned this Apr 12, 2021
@IlanZuckerman
Copy link
Collaborator Author

Tested and deployed on prod env. the web page is up and running.

lmilbaum
lmilbaum suggested changes Apr 12, 2021
View reviewed changes
Dockerfile Outdated
WORKDIR /code
COPY . .
RUN pip3 install -r requirements.txt
FROM quay.io/redhat-beyond/beyond
EXPOSE 5000

ENTRYPOINT python3 app.py runserver > web-application.log
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this Dockerfile used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dockerfile is used by docker-compose in both of the following cases:

  1. In the github flow, it is being invoked from here:

.github/workflows/main.yml
- name: Build the docker-compose stack
run: docker-compose up -d

  1. In the Vagrant flow (dev, stage, prod) it is being invoked from here:

Vagrantfile
config.vm.provision :docker_compose, yml: "/vagrant/docker-compose.yml", rebuild: true, run: "always",
compose_version: "1.28.2"

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the code. There seems to be a circle dependency. docker-compose is building the same images as the parent image.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You Are right. I was confused.
There is no need for 'Docker' file anymore. Because the 'beyond' image is now being pulled from quayi.
I am removing the Docker file.

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if you want to update the image? Is it going to be done manually?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well, I assume that this is going to be a different flow for building and pushing the image...
Can you suggest how should I start this flow?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please google a bit? Take a look at my other comments please.

lmilbaum
lmilbaum suggested changes Apr 13, 2021
View reviewed changes
Dockerfile Outdated
WORKDIR /code
COPY . .
RUN pip3 install -r requirements.txt
FROM quay.io/redhat-beyond/beyond
EXPOSE 5000

ENTRYPOINT python3 app.py runserver > web-application.log
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the code. There seems to be a circle dependency. docker-compose is building the same images as the parent image.

docker-compose.yml
@@ -1,7 +1,7 @@
version: '3.9'
services:
beyond:
build: .
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was the build key removed?

Copy link
Collaborator Author

@IlanZuckerman IlanZuckerman Apr 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Because the image build stage is now removed from this flow.
The image is ready and being hosted on quayei already. We just need to pull it and thats it.

From $ vagrant up stage --provider=aws :
.
.
.
==> stage: Pulling beyond (quay.io/redhat-beyond/beyond:)...
stage: latest: Pulling from redhat-beyond/beyond

Copy link
Contributor

Choose a reason for hiding this comment

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

Something is wrong with this concept. An automatic build should happen when a code change lands in master.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, there should be a different flow for building and pushing the image. lets discuss this in my reply on your comment from above (to avoid duplicated messages)

lmilbaum
lmilbaum previously requested changes Apr 13, 2021
View reviewed changes
Copy link
Contributor

@lmilbaum lmilbaum left a comment

Choose a reason for hiding this comment

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

Please read my last comments. The whole approach is wrong.
Can you please put your concept in writing? Let's agree on the concept BEFORE the implementation.

@IlanZuckerman
Copy link
Collaborator Author

Please read my last comments. The whole approach is wrong.
Can you please put your concept in writing? Let's agree on the concept BEFORE the implementation.

Why it is all wrong? We said to decouple, so i decoupled the flow. Now it only pulls the ready image.
I agree that we still lack the second part - which is about updating / rebuilding the image and pushing it back to the repo. I just dont have a clue right now how and where this should be combined in the project.

@IlanZuckerman IlanZuckerman force-pushed the decouple_image_build branch 3 times, most recently from 4221dbd to 5e0afa5 Compare April 16, 2021 11:23
@IlanZuckerman
Copy link
Collaborator Author

@lioramilbaum Could you please add 'GitHub Secrets' to your 'https://github.com/beyond-io/beyond/' project?
Or maybe give me the permissions to do that?

I am following this example i found to integrate the github image build into the github actions:
https://github1s.com/HunterGerlach/image-to-quay-with-github-actions

In the example, there is a 'build image' + 'push image' to quay when 'commit' was found on the github repo.
I think this is the way to do this right. So I started implementing it in our .github/workflows/main.yml

@lmilbaum
Copy link
Contributor

@lioramilbaum Could you please add 'GitHub Secrets' to your 'https://github.com/beyond-io/beyond/' project?
Or maybe give me the permissions to do that?

I am following this example i found to integrate the github image build into the github actions:
https://github1s.com/HunterGerlach/image-to-quay-with-github-actions

In the example, there is a 'build image' + 'push image' to quay when 'commit' was found on the github repo.
I think this is the way to do this right. So I started implementing it in our .github/workflows/main.yml

I have elevated your permissions to Admin. You should be able to add secrets from now on and going forward. I didn't take the time to review the approach you are driving. I still think that dumping the suggested approach into a doc/issue and agreeing on the direction would save you and I time. Please consider that. I do not know if I would want to invest another review cycle on a wrong approach.

@IlanZuckerman IlanZuckerman force-pushed the decouple_image_build branch 15 times, most recently from 3e782b2 to cbb44eb Compare April 21, 2021 09:20
@IlanZuckerman IlanZuckerman force-pushed the decouple_image_build branch 25 times, most recently from 26d6131 to 247ff57 Compare April 28, 2021 12:28
@IlanZuckerman IlanZuckerman dismissed lmilbaum’s stale review April 28, 2021 12:56

Need to test 'push' action for the new build push pipeline. will revert it afterwards

@IlanZuckerman IlanZuckerman merged commit b4799d2 into redhat-beyond:master Apr 28, 2021
@IlanZuckerman IlanZuckerman mentioned this pull request Apr 28, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmilbaum lmilbaum lmilbaum left review comments

Assignees

@IlanZuckerman IlanZuckerman

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Decouple the process of image build from the current flow
2 participants
@IlanZuckerman @lmilbaum