Skip to content
This repository has been archived by the owner on May 6, 2024. It is now read-only.

feat(docker): Adds a cloudbuild file to publish the docker image #34

Merged
merged 21 commits into from
Apr 14, 2021

Conversation

fpacifici
Copy link
Contributor

@fpacifici fpacifici commented Apr 1, 2021

This adds a new cloudbuild file to create the docker image and publish it on the public repo.

Specifically:

  • It does not remove the old cloudbuild so we can merge the new one for the public repo, have the first image published and, once that is verified to work properly remove the old one. During this process it is desirable to preserve the old file in a functioning state to be able to apply a fix and do a deployment if needed.
  • It adds a docker compose file to let cloudbuild run the unit tests before publishing the image.

@fpacifici fpacifici changed the title feat(docker) Publish docker image feat(docker) Adds a cloudbuild file to publish the docker image Apr 2, 2021
@fpacifici fpacifici requested review from a team and BYK April 2, 2021 00:56
@fpacifici fpacifici marked this pull request as ready for review April 2, 2021 00:56
Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

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

I have two concerns:

  1. I don't understand why we don extend the existing cloudbuild.yaml file instead of introducing a new one (and adding new triggers).
  2. I don't think the testing we do here is efficient or useful as we should be running unit tests on GitHub Actions anyway, right?

Copy link
Contributor Author

@fpacifici fpacifici left a comment

Choose a reason for hiding this comment

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

Thanks, will address the comment soon

I don't understand why we don extend the existing cloudbuild.yaml file instead of introducing a new one (and adding new triggers).

To my knowledge if I extend the existing one and then call it from two triggers (one is the one we use today to generate the internal image (this repo was used to be private) and the other would be a new one on the public repo), The private one would try to publish the image on the public repo. I don't think that would work.

Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

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

To my knowledge if I extend the existing one and then call it from two triggers (one is the one we use today to generate the internal image (this repo was used to be private) and the other would be a new one on the public repo), The private one would try to publish the image on the public repo. I don't think that would work.

Not sure if I fully grasp what you have in mind here so will do a Slack sync before doing another pass on this PR. Thanks a lot for all the updates and providing additional context!

@fpacifici
Copy link
Contributor Author

I tested the cloudbuild file and plugged it whenever a PR is updated and a commit on master happens.
It now works: https://console.cloud.google.com/cloud-build/builds/d361cbdf-9bf1-4c6c-9eb5-789d3126ab54?project=sentryio

Also removed the build step of the cdc image from the e2e test. Now it uses the same image we are going to push to the registry even to execute the test script.

@fpacifici fpacifici requested a review from BYK April 5, 2021 23:24
@BYK BYK changed the title feat(docker) Adds a cloudbuild file to publish the docker image feat(docker): Adds a cloudbuild file to publish the docker image Apr 12, 2021
'SOURCE_COMMIT=$COMMIT_SHA',
'--destination=us.gcr.io/$PROJECT_ID/$REPO_NAME:$COMMIT_SHA',
'-f', './Dockerfile',
'--target=application'
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is obsolete now as we use the same image for testing and just install pytest at container start?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually no, this is still needed for the CI github actions

Copy link
Member

Choose a reason for hiding this comment

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

I'd really like to remove the multi-stage Docker file, at least for testing image.

Copy link
Contributor Author

@fpacifici fpacifici Apr 15, 2021

Choose a reason for hiding this comment

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

Ok, I can split it into two files. What's the reasoning for preferring two files too a multi stage Dockerfile? I thought multi stage was meant for use cases like this.

@fpacifici fpacifici merged commit 748cb15 into master Apr 14, 2021
@BYK BYK deleted the feat/add_gcb branch April 15, 2021 08:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants