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

WIP - Improve docker images for faster build and deploy #563

Closed
wants to merge 17 commits into from

Conversation

ahangarha
Copy link
Member

@ahangarha ahangarha commented Oct 29, 2023

Creating a base Dockerfile for this project so that it can be used in .controlplane/Dockerfile to speed up the build time.

  • This file is based on Rails 7.1 Dockerfile.
  • The Docker base image should be built from Dockerfile_base and pushed to the Docker hub.
  • For deploying on Control Plane, we should use .controlplane/Dockerfile_from_base. For this, we need to uncomment the line for the custom Dockerfile path in .controlplane/controlplane.yml file.

These new Dockerfiles are made originally with the idea of having a way to make a faster build/deploy process on cpln. We still need the existing Dockerfiles to have more control and flexibility when experimenting with deployment.

Result:

Screenshot from 2023-12-09 19-36-43-e


This change is Reviewable

Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

Why are we adding the root docker file?

How does it relate to the .controlplane/Dockerfile?

@rafaelgomesxyz @ahangarha @Judahmeek

if [ "${1}" == "./bin/rails" ] && [ "${2}" == "server" ]; then
echo " -- Preparing database"
./bin/rails db:prepare
fi
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@ahangarha why is this duplicated in the bin script in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

The bin/docker-entrypoint file comes from new Rails setup.

@ahangarha
Copy link
Member Author

@justin808
My thought was this:

  • We make a dockerfile at the project's root, which is the base for building the image for this project. We push this image to the docker hub.
  • We use the dockerfile in .controlplane to use this image from docker hub and prepare the image for deployment on Control Plane.

Dockerfile Outdated

# Install packages needed for deployment
RUN apt-get update -qq && \
apt-get install --no-install-recommends -y curl libvips postgresql-client && \
Copy link
Member

Choose a reason for hiding this comment

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

Why is postgresql-client installed? I tried to find references to psql and couldn't find it.

Copy link
Member Author

Choose a reason for hiding this comment

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

It comes from Rails new Dockerfile.

Dockerfile Outdated
COPY . ./
# Copy built artifacts: gems, application
COPY --from=build /usr/local/bundle /usr/local/bundle
COPY --from=build /app /app
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth it having a separate build step image vs a single one where you delete the unneeded stuff afterward? You already did some cleaning on line 37.

Copy link
Member Author

@ahangarha ahangarha Dec 9, 2023

Choose a reason for hiding this comment

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

I just followed the Dockerfile proposed by Rails.
It might still be faulty, but it follows the same structure.

@ahangarha ahangarha marked this pull request as ready for review December 9, 2023 17:01
@ahangarha ahangarha changed the title WIP - Improve docker image Improve docker image Dec 9, 2023
@ahangarha ahangarha changed the title Improve docker image Improve docker images for faster build and deploy Dec 9, 2023
@justin808
Copy link
Member

@ahangarha and @Judahmeek how much testing did you do of this?

@ahangarha
Copy link
Member Author

@justin808

how much testing did you do of this?

I have tested multiple times. The last time, I made sure no image was left on my system and built from scratch (both the base image on the docker hub, and the image to be deployed on cpln)

Have you noticed any issues?

@justin808 justin808 marked this pull request as draft December 15, 2023 20:32
@@ -0,0 +1,17 @@
FROM ahangarha/rwrt-base:latest
Copy link
Member

Choose a reason for hiding this comment

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

WTFs:

  1. personal name.
  2. need comments on why file exists and how to use

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason behind this was that before pushing this base image to the official docker hub of the company, I could demonstrate deployment. I needed a review on the implementation of this Docker base image. Now I got it and now I can move forward.

@ahangarha ahangarha changed the title Improve docker images for faster build and deploy WIP - Improve docker images for faster build and deploy Dec 15, 2023
@@ -1,4 +1,4 @@
#!/bin/sh
#!/bin/bash -e
Copy link
Member

Choose a reason for hiding this comment

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

why the -e flag?

Copy link
Member Author

@ahangarha ahangarha Dec 25, 2023

Choose a reason for hiding this comment

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

I applied it based on the new Rails 7.1 Dockerfile. It is needed for the next changes to run; the == operation in the condition.

It is to make the whole Dockerfiles have similar content, making it easier to debug issues in the future.

Update:
Actually, bash is required for the == operator.

The -e option is used to exit running the script immediately after a command returns with a non-zero status.

# image from Docker hub (shakacode/rwrt-base:latest). To this end, uncomment
# the line for the custom dockerfile in the controlplane.yml file.

FROM shakacode/rwrt-base:latest
Copy link
Member

Choose a reason for hiding this comment

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

This is going to confuse people.

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand.
Perhaps we can move these files into .shakacode folder which will be the place for keeping files only related to our development tasks.

@justin808
Copy link
Member

Reopen new PR if any of this is still useful.

@justin808 justin808 closed this Dec 28, 2023
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.

None yet

3 participants