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

refactor(atlantis-image): recombine with atlantis-base #3001

Merged

Conversation

GenPage
Copy link
Member

@GenPage GenPage commented Jan 18, 2023

Atlantis built and maintained two separate docker images, atlantis and atlantis-base. This made cutting releases cumbersome and had inefficiencies in the docker image layers.

This PR condenses the workflow into a single job and Dockerfile once again by utilizing multi-stage builds more heavily and Docker build targeting.

what

  • Removes docker-base files and GHA workflow
  • Streamlines additional alpine/debian build images in one dockerfile
  • targets alpine and debian images via a single workflow and image type matrix
  • improve docker image layering for cache busting.

why

  • had to release atlantis-base manually on to bring package updates into main image
  • inefficient as it would bust cache for the whole atlantis image
  • additional improvements and standardized how we download binary dependencies and copy them into final image

tests

  • I have tested my changes by:
    • locally running the docker build commands:
      • DOCKER_BUILDKIT=1 docker build -f Dockerfile --platform linux/amd64 --target alpine -t atlanits-alpine .
      • DOCKER_BUILDKIT=1 docker build -f Dockerfile --platform linux/amd64 --target debian -t atlanits-debian .
    • Additional testing with the GHA workflow is needed

references

@GenPage GenPage force-pushed the ci/refactor-atlantis-image-building branch from 8769a21 to 4d29519 Compare January 18, 2023 15:47
@GenPage GenPage marked this pull request as ready for review January 18, 2023 16:03
@GenPage GenPage requested a review from a team as a code owner January 18, 2023 16:03
@jamengual
Copy link
Contributor

jamengual commented Jan 18, 2023

how will the update flow look like?

if we need to update ARG ALPINE_TAG=3.17.0 ARG DEBIAN_TAG=bullseye-20221219-slim

or add a TF version?

@GenPage
Copy link
Member Author

GenPage commented Jan 18, 2023

how will the update flow look like?

if we need to update ARG ALPINE_TAG=3.17.0 ARG DEBIAN_TAG=bullseye-20221219-slim

or add a TF version?

The same way it is right now, you'll update the Dockerfile. I purposefully left the image tags as ARGs and not ENVs incase we decide on a solution to pass in tags via the GHA workflow.

Dockerfile.dev Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Copy link
Member

@nitrocode nitrocode left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution. I had a few comments

Dockerfile Outdated Show resolved Hide resolved
@GenPage GenPage force-pushed the ci/refactor-atlantis-image-building branch from 4d29519 to 39e6484 Compare January 18, 2023 22:05
@GenPage GenPage added build Relating to how we build Atlantis refactoring Code refactoring that doesn't add additional functionality area/build docker Pull requests that update Docker code labels Jan 18, 2023
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated
RUN apk add --no-cache \
ca-certificates~=20220614 \
curl~=7.87 \
git~=2.38 \
Copy link
Member

Choose a reason for hiding this comment

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

Does git~=2.38 allow the latest git 2.39 to be installed without using the separate package repo?

Ref: #2998

Copy link
Member Author

@GenPage GenPage Jan 23, 2023

Choose a reason for hiding this comment

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

I doesn't appear to: https://jubianchi.github.io/semver-check/#/~%3D2.38/2.39.0

Looks like we might have to do git~=2 which honestly is weird to me.

Copy link
Member

Choose a reason for hiding this comment

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

Seems ok to me, as long as it picks up the latest git security patch. If you're not comfortable with ~2, let's raise the pin to ~2.39.

@nitrocode nitrocode requested a review from jamengual January 23, 2023 03:09
@GenPage GenPage force-pushed the ci/refactor-atlantis-image-building branch from b63bc34 to db22d76 Compare January 23, 2023 04:59
@GenPage GenPage enabled auto-merge (squash) January 23, 2023 04:59
# Install packages needed for building/verifying dependencies
# hadolint ignore=DL3008,SC2261
WORKDIR /tmp/build
RUN apt-get update \
Copy link
Member

Choose a reason for hiding this comment

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

Why are these lines repeated on 180? Couldn't we install these packages just once in the debian release?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I can improve this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Revisiting this, we re-install twice because we throw away the builder image which has more package installs, and this is a slimmed down install for the actual release image.

Copy link
Member

Choose a reason for hiding this comment

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

But now we have 2 places where we need to make sure these dependencies are up to date... Seems like extra maintenance and an easy thing to bump in one section and forget to bump in another

Dockerfile Show resolved Hide resolved
@nitrocode nitrocode dismissed their stale review January 27, 2023 23:37

deferring to pepe on this pr

@GenPage GenPage disabled auto-merge April 4, 2023 21:46
@GenPage GenPage enabled auto-merge (squash) April 4, 2023 21:46
@GenPage GenPage merged commit 310aff8 into runatlantis:main Apr 4, 2023
@GenPage GenPage deleted the ci/refactor-atlantis-image-building branch April 4, 2023 21:49
@GenPage GenPage restored the ci/refactor-atlantis-image-building branch April 4, 2023 21:55
GenPage added a commit that referenced this pull request Apr 4, 2023
@@ -56,6 +63,7 @@ jobs:
images: |
${{ env.DOCKER_REPO }}
labels: |
org.opencontainers.image.authors="Anubhav Mishra, Luke Kysow"
Copy link
Member

@nitrocode nitrocode Apr 4, 2023

Choose a reason for hiding this comment

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

nit: This label seems no longer relevant as these authors are no longer actively maintaining the repo. I think we can omit this in the future

context: .
platforms: linux/arm64/v8,linux/amd64,linux/arm/v7
push: ${{ github.event_name != 'pull_request' }}
tags: |
Copy link
Member

Choose a reason for hiding this comment

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

This seems incorrect. We started using the metadata tags in a recent pr.

Copy link
Member

Choose a reason for hiding this comment

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

I think this entire block can be removed

Copy link
Member

Choose a reason for hiding this comment

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

SHELL ["/bin/bash", "-o", "pipefail", "-c"]
RUN AVAILABLE_TERRAFORM_VERSIONS="1.0.11 1.1.9 1.2.9 1.3.9 ${DEFAULT_TERRAFORM_VERSION}" && \
# In the official Atlantis image, we only have the latest of each Terraform version.
RUN AVAILABLE_TERRAFORM_VERSIONS="1.1.9 1.2.9 1.3.9 ${DEFAULT_TERRAFORM_VERSION}" && \
Copy link
Member

Choose a reason for hiding this comment

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

Why was the shell argument removed?

@@ -1,7 +1,3 @@
FROM ghcr.io/runatlantis/atlantis:latest
COPY atlantis /usr/local/bin/atlantis
Copy link
Member

Choose a reason for hiding this comment

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

From the comments below, I don't think this image is needed anymore.

@jamengual

openssl>=1.1.1n

# install conftest
# renovate: datasource=github-releases depName=open-policy-agent/conftest
Copy link
Member

Choose a reason for hiding this comment

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

This renovate comment should be directly before the conftest line

@jamengual @GenPage

@nitrocode
Copy link
Member

@GenPage @jamengual could you folks give this one more review and please put in a follow up to address the above comments

@nitrocode
Copy link
Member

Also it looks like this pr was merged without approval. Im unsure how that happened.

@lkysow please check repo settings when time permits. I think we need to remove pr approvers when new commits are pushed

@nitrocode nitrocode added this to the v0.23.4 milestone Apr 5, 2023
wyardley added a commit to wyardley/atlantis that referenced this pull request Apr 11, 2023
It appears that when runatlantis#3001 combined the Dockerfiles, the bit that adds
the `atlantis` user was not added to the Debian build stage

Also correct some trailing spaces and missing EOL in
`docker-compose.yml`
wyardley added a commit to wyardley/atlantis that referenced this pull request Apr 11, 2023
It appears that when runatlantis#3001 combined the Dockerfiles, the bit that adds
the `atlantis` user was not added to the Debian build stage

Also correct some trailing spaces and missing EOL in
`docker-compose.yml`
wyardley added a commit to wyardley/atlantis that referenced this pull request Apr 11, 2023
It appears that when runatlantis#3001 combined the Dockerfiles, the bit that adds
the `atlantis` user was not added to the Debian build stage

Also correct some trailing spaces and missing EOL in
`docker-compose.yml`
GenPage pushed a commit to wyardley/atlantis that referenced this pull request Apr 11, 2023
It appears that when runatlantis#3001 combined the Dockerfiles, the bit that adds
the `atlantis` user was not added to the Debian build stage

Also correct some trailing spaces and missing EOL in
`docker-compose.yml`
GenPage pushed a commit to wyardley/atlantis that referenced this pull request Apr 11, 2023
It appears that when runatlantis#3001 combined the Dockerfiles, the bit that adds
the `atlantis` user was not added to the Debian build stage

Also correct some trailing spaces and missing EOL in
`docker-compose.yml`
GenPage pushed a commit that referenced this pull request Apr 11, 2023
It appears that when #3001 combined the Dockerfiles, the bit that adds
the `atlantis` user was not added to the Debian build stage

Also correct some trailing spaces and missing EOL in
`docker-compose.yml`
nitrocode added a commit that referenced this pull request May 5, 2023
* refactor: atlantis-image build pipeline and docker images

Atlantis built and maintained two separate docker images, atlantis and
atlantis-base. This made cutting releases cumbersome and had
inefficiencies in the docker image layers.

This PR condenses the workflow into a single job and Dockerfile once
again by utilizing multi-stage builds more heavily and Docker build targeting.

* fix: apply @nitrocode suggestions from code review
* feat: hadolint
* fix: DL4006 on go mod graph
* fix: version regressions

---------

Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
nitrocode pushed a commit that referenced this pull request May 5, 2023
It appears that when #3001 combined the Dockerfiles, the bit that adds
the `atlantis` user was not added to the Debian build stage

Also correct some trailing spaces and missing EOL in
`docker-compose.yml`
ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
)

* refactor: atlantis-image build pipeline and docker images

Atlantis built and maintained two separate docker images, atlantis and
atlantis-base. This made cutting releases cumbersome and had
inefficiencies in the docker image layers.

This PR condenses the workflow into a single job and Dockerfile once
again by utilizing multi-stage builds more heavily and Docker build targeting.

* fix: apply @nitrocode suggestions from code review
* feat: hadolint
* fix: DL4006 on go mod graph
* fix: version regressions

---------

Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
It appears that when runatlantis#3001 combined the Dockerfiles, the bit that adds
the `atlantis` user was not added to the Debian build stage

Also correct some trailing spaces and missing EOL in
`docker-compose.yml`
ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
)

* refactor: atlantis-image build pipeline and docker images

Atlantis built and maintained two separate docker images, atlantis and
atlantis-base. This made cutting releases cumbersome and had
inefficiencies in the docker image layers.

This PR condenses the workflow into a single job and Dockerfile once
again by utilizing multi-stage builds more heavily and Docker build targeting.

* fix: apply @nitrocode suggestions from code review
* feat: hadolint
* fix: DL4006 on go mod graph
* fix: version regressions

---------

Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
It appears that when runatlantis#3001 combined the Dockerfiles, the bit that adds
the `atlantis` user was not added to the Debian build stage

Also correct some trailing spaces and missing EOL in
`docker-compose.yml`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Relating to how we build Atlantis docker Pull requests that update Docker code github-actions refactoring Code refactoring that doesn't add additional functionality waiting-on-response Waiting for a response from the user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants