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

(feat) Upload images to ECR #179

Merged
merged 24 commits into from
Feb 23, 2022
Merged

Conversation

tdimnet
Copy link
Contributor

@tdimnet tdimnet commented Oct 19, 2021

Hi,

this is related to a discussion I had with Kirk. AWS wanted to use our Docker Images but they prefer using them on ECR instead of DockerHub.

I updated all of the Github actions in order to make them push the images to both DockerHub and ECR.
I also updated the release script and created the ECR Repo.

Thomas Dimnet added 3 commits October 19, 2021 16:25
- Configure AWS and log into ECR
- Push the images to ECR
- Discount is now pointing to the correct repo
- Storefront fixed is now using the correct branch
Copy link
Collaborator

@arapulido arapulido left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I added some comments and questions.

.github/workflows/advertisements-errors.yml Outdated Show resolved Hide resolved
.github/workflows/advertisements-errors.yml Show resolved Hide resolved
.github/workflows/advertisements-errors.yml Outdated Show resolved Hide resolved
.github/workflows/advertisements-fixed.yml Outdated Show resolved Hide resolved
.github/workflows/advertisements.yml Outdated Show resolved Hide resolved
.github/workflows/discounts.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
- I was pointing to wrong ECR repo
- Update workflow
@tdimnet
Copy link
Contributor Author

tdimnet commented Oct 21, 2021

Thank you @arapulido for your review.
What did you have in mind for the release section?

By the way, I should also update the doc to let people know about ECR?

Copy link
Collaborator

@arapulido arapulido left a comment

Choose a reason for hiding this comment

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

There is an image name that has changed its name

.github/workflows/discounts-fixed.yml Show resolved Hide resolved
@arapulido
Copy link
Collaborator

What did you have in mind for the release section?

what do you mean by this?

Copy link
Collaborator

@arapulido arapulido left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

Copy link
Collaborator

@stanleegoodspeed stanleegoodspeed left a comment

Choose a reason for hiding this comment

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

Hi @tdimnet , what AWS account owns these ECR repositories?

@tdimnet
Copy link
Contributor Author

tdimnet commented Jan 17, 2022

Hey @stanleegoodspeed!

I send you the name of the account via Slack.


And then, I'll merge this PR, it's been open for so long.

Copy link
Collaborator

@stanleegoodspeed stanleegoodspeed left a comment

Choose a reason for hiding this comment

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

  • I confirmed that the github workflows can push to the new ECR registries under the dd-marketing account in addition to the existing repos in Dockerhub (tested against the nginx workflow + repos)
  • I confirmed that the images can be pulled as expected from ECR using the following docker compose service definition running locally:
 nginx:
    restart: always
    image: public.ecr.aws/x2b9z2t7/ddtraining/nginx:latest
    ports:
      - "80:80"
    depends_on:
      - frontend
    labels:
      com.datadoghq.ad.logs: '[{"source": "nginx", "service": "nginx"}]'

Remaining Question

  • traffic-replay is not included in the current release workflow and therefore has not been included in the new workflow. Is that intended?

Copy link
Contributor

@burningion burningion left a comment

Choose a reason for hiding this comment

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

I comment on some (but not all!) places where the dockerhub pushes were occuring. Let's ensure we're only pushing the the ECR registry with our automated builds.

.github/workflows/advertisements.yml Outdated Show resolved Hide resolved
.github/workflows/attackbox.yml Outdated Show resolved Hide resolved
.github/workflows/discounts-fixed.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
@stanleegoodspeed stanleegoodspeed merged commit 6a6f4b3 into main Feb 23, 2022
@stanleegoodspeed stanleegoodspeed deleted the tdimnet/from-dockerhub-to-ecr branch February 23, 2022 15:21
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

4 participants