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

Infra: Add actions to publish to ECR & Docker Hub #347

Merged
merged 10 commits into from
Oct 7, 2024

Conversation

giom-l
Copy link
Contributor

@giom-l giom-l commented May 3, 2024

What changes did you make? (Give an overview)
Add login to dockerhub and push image to it in main workflow
Fixes #237
Fixes #242

Is there anything you'd like reviewers to focus on?
Secrets names may need to be changed based on what is currently available (no visibility on it)

How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)

  • Yes, on my forked project with own credentials

Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)

  • I have performed a self-review of my own code

Check out Contributing and Code of Conduct

image

@giom-l giom-l requested a review from a team as a code owner May 3, 2024 11:00
@kapybro kapybro bot added status/triage Issues pending maintainers triage status/triage/manual Manual triage in progress status/triage/completed Automatic triage completed and removed status/triage Issues pending maintainers triage labels May 3, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hi giom-l! 👋

Welcome, and thank you for opening your first PR in the repo!

Please wait for triaging by our maintainers.

Please take a look at our contributing guide.

Copy link
Member

@Haarolean Haarolean left a comment

Choose a reason for hiding this comment

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

Hi, could you make this a separate job (within the same workflow) please? So we don't have to rebuild (or re-push) everything if one of the things fail.
Ideally:

  • build
  • ghcr push
  • dockerhub push
  • aws ecr push

You'd need to transfer artifacts between the jobs, feel free to use .github/workflows/e2e-*.yml workflows as reference.

@giom-l
Copy link
Contributor Author

giom-l commented May 7, 2024

Hi,
I spent some time last days to on that and mostly struggled with my own station to have quicker local builds...
Anyway I also took into account #349 to go with a solution that would support it.

So here are 2 versions that work the same, let me know which one your prefer.
In both, I had to activate containerd as builder, for some different reasons (being able to load multi platforms images & preserve provenance attestations)

With docker save / docker load mechanism :
https://github.com/giom-l/kafka-ui/blob/feat/improve_docker_publish/.github/workflows/save_load.yml

With containerd all along (using oci output & ctr to reuse the produced image)
https://github.com/giom-l/kafka-ui/blob/feat/improve_docker_publish/.github/workflows/with_ctr.yml

All produced images can be found in all 3 repositories :

For ECR, my workflow uses OIDC provider .
It would be the same with classic credentials (I saw some in your other workflows) and even be less verbose by using this action that would allow something like

- name: Push to ECR
  id: ecr
  uses: jwalton/gh-ecr-push@v2
  with:
    access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }}
    secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
    region: us-east-1
    local-image: kafka-ui:temp
    image: <repo>/kafka-ui:main, <repo>/kafka-ui:<sha>

@Haarolean Haarolean self-assigned this May 7, 2024
@Haarolean Haarolean added type/enhancement En enhancement/improvement to an already existing feature scope/infra CI, CD, dev. env, etc. and removed status/triage/manual Manual triage in progress labels May 7, 2024
@Haarolean
Copy link
Member

Hi, I spent some time last days to on that and mostly struggled with my own station to have quicker local builds... Anyway I also took into account #349 to go with a solution that would support it.

So here are 2 versions that work the same, let me know which one your prefer. In both, I had to activate containerd as builder, for some different reasons (being able to load multi platforms images & preserve provenance attestations)

With docker save / docker load mechanism : https://github.com/giom-l/kafka-ui/blob/feat/improve_docker_publish/.github/workflows/save_load.yml

With containerd all along (using oci output & ctr to reuse the produced image) https://github.com/giom-l/kafka-ui/blob/feat/improve_docker_publish/.github/workflows/with_ctr.yml

All produced images can be found in all 3 repositories :

For ECR, my workflow uses OIDC provider . It would be the same with classic credentials (I saw some in your other workflows) and even be less verbose by using this action that would allow something like

- name: Push to ECR
  id: ecr
  uses: jwalton/gh-ecr-push@v2
  with:
    access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }}
    secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
    region: us-east-1
    local-image: kafka-ui:temp
    image: <repo>/kafka-ui:main, <repo>/kafka-ui:<sha>

@giom-l thank you for the analysis. I'd prefer using docker save/load mechanism to have the same approach between different workflows (for example, e2e-run.yml, already uses this approach).

Can you update the PR branch with this approach so we can proceed with the review? Thank you

@Haarolean Haarolean assigned giom-l and unassigned Haarolean May 19, 2024
@giom-l
Copy link
Contributor Author

giom-l commented May 20, 2024

Sorry for the delay !
I updated the PR using the save/load mechanism as requested.
I also impacted the release workflow that I forgot initially.

For the ECR publish, it's still "WIP" as I would need some informations about how you want to authenticate and where (ecr public ?)

@giom-l giom-l requested a review from Haarolean May 22, 2024 10:07
@Haarolean
Copy link
Member

Haarolean commented May 22, 2024

I updated the PR using the save/load mechanism as requested.

Thank you.

about how you want to authenticate

I guess, we could auth the same way we do here (unless you have a better suggestion?)

and where (ecr public ?)

Yes, as we (accidentally) had one before (source).

Can I also ask you to refactor the workflows to use reusable workflows to reduce the copypaste in these workflows? I can take a look myself later as an alternative.

@giom-l
Copy link
Contributor Author

giom-l commented May 23, 2024

Sure.
Just a question about how you see that :)
One reusable action per target registry (ecr, github, ecr) ?
Or one reusable action (something like image-publish) that push to all registries (but I'm not sure how it behave if one of them fails. Can we still rerun just a part of it ?) ?

I thought about one generic reusable action that will take parameters (like registry, credentials) but since ECR login is not the same as the other, I'm not sure it's doable.

Let me know what you think

@Haarolean
Copy link
Member

Haarolean commented May 23, 2024

I see that as follows:

  1. A workflow to run on a push to main
  2. A workflow to run on a release
  3. Both 1) and 2) call another workflow to build an image (docker-build.yml) and a workflow to publish one (docker-publish.yml)
  4. docker-publish.yml contains 3 jobs for each registry (gh, ecr, dockerhub)
    In this case we can extract common parts for each workflow and keep publishing as separate jobs within a separate calling workflow.

Feel free to use e2e-*.yml workflows as templates, we have the basic required stuff there (calling other workflows, passing arguments through, transferring artifacts between jobs, etc.)

Can we still rerun just a part of it

Yes, we would be able to rerun separate jobs within a workflow like this:
image

@giom-l giom-l force-pushed the feat/add_dockerhub_publish branch from 4d38eb9 to f3f57d8 Compare May 27, 2024 10:49
@giom-l
Copy link
Contributor Author

giom-l commented May 27, 2024

Hello

I gave this some time yesterday and made it work as follows (PR udpated) :

  • 1 docker_build.yml workflow
  • 1 docker_publish.yml workflow, which itself calls 3 differents workflows :
    • 1 publish_ecr.yml
    • 1 publish_dockerhub.yml
    • 1 publish_ghcr.yml

About extracting common parts for the three publish jobs :
It seems not really useful to me since it's only a matter of downloading artifacts & configuring docker daemon, which can't be shared between jobs.

However, I also experienced another solution that can be found in this branch :
The main & release job will call the docker_publish and this one is more generic (only the logging part isn't) and uses matrix instead of calling others workflows.
I find this solution to be more elegant (less duplicated code, no intermediary workflow)

Let me know what you think and I'll finalize the PR with the solution that is preferred on your side.

@giom-l giom-l force-pushed the feat/add_dockerhub_publish branch from 48df758 to b02611a Compare May 27, 2024 13:53
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/publish_dockerhub.yml Outdated Show resolved Hide resolved
@Haarolean
Copy link
Member

Hey, sorry for the delay.

One more thing, can we get separate workflows like publish_x.yml to be separate jobs within one workflow instead? I don't see any point in making them separate workflows rather than jobs, it just adds additional overhead. Let me know what you think.

@giom-l
Copy link
Contributor Author

giom-l commented Jun 6, 2024

Hello,

Sure, I'll give some tests to your proposals and fix them if everything's good.

About

One more thing, can we get separate workflows like publish_x.yml to be separate jobs within one workflow instead? I don't see any point in making them separate workflows rather than jobs, it just adds additional overhead. Let me know what you think.

I totally agree.
This is why I also proposed another solution that I find more elegant.
You can find the workflow here

It uses matrix strategy to produce as much jobs as we want, but it remains in 1 workflow only (way more readable).
The downside is about having steps that will run only in certain cases (login to target platform) but it removes duplication of code.

Let me know if it better fits what you expect and I'll port it to the PR branch :) .

@Haarolean
Copy link
Member

Haarolean commented Jun 9, 2024

Hi,

the problem with the matrix approach is that it's just one job within a workflow which will result in unnecessary re-deployments in case when one of the three deployments fails (say, we've deployed to ecr and then ghcr has failed). If we split them in three separate jobs, we won't have to redeploy on job restart. Can we adjust that?

image image image

@giom-l
Copy link
Contributor Author

giom-l commented Jun 9, 2024

Hello,

I'm having the same behavior between 3 jobs and a matrix that generates 3 jobs.
See here, I changed a secret on my project to make the dockerhub publish fail (which happened)

image

If I rerun all jobs, all of them will rerun (of course)
image

But if I rerun the failed one, only failed one will rerun.
image

One can also rerun the failed job with the dedicated icon
image
or after selecting the failed job.
image

And you can see here that the previously succeeded jobs didn't rerun when the failed one restart
image

Or am I missing your point ?
In any case, if you confirm you prefer 3 separates jobs in the same workflow (with duplicated code though), that's fine for me too :)

.github/workflows/publish_ghcr.yml Outdated Show resolved Hide resolved
.github/workflows/publish_ecr.yml Outdated Show resolved Hide resolved
@Haarolean
Copy link
Member

I'm having the same behavior between 3 jobs and a matrix that generates 3 jobs.

Oh, thanks for correcting me! I've seen matrix workflows in action but haven't used them personally before so I thought the part we need was missing :)

Besides the minor comments I've left (and other possible things to clean up you manage to find) I'd need to get our credentials for ECR and we can merge. Thank you very much for the high-quality PR and continuous cooperation, and I am sorry for the delays on my part :)

@giom-l giom-l force-pushed the feat/add_dockerhub_publish branch from 6106bd6 to f2a6504 Compare June 18, 2024 07:05
@giom-l
Copy link
Contributor Author

giom-l commented Jun 18, 2024

Hello,

I just updated the PR to use matrix workflows and removed old workflows that contained some hardcoded strings.

It should work with no issue as soon as :

  • The secrets used in the workflows are correct (let me know if I need to change some names)
secrets.DOCKERHUB_USERNAME
secrets.DOCKERHUB_TOKEN
  • You are using ECR public and not private.
  • AWS authentication is handled with an AWS role
secrets.AWS_ROLE

This is a personal preference to use role over static credentials.
If you prefer to stick with credentials, it will be easy to change. Let me know.

  • The owner/repository names are the same for all 3 registries (kafbat/kafka-ui)
    If that is not the case, we'll need to introduce another variable "OWNER" in the docker_publish.yml workflow to take care of this difference.

@giom-l giom-l requested a review from Haarolean June 18, 2024 07:13
.github/workflows/docker_build.yml Outdated Show resolved Hide resolved
.github/workflows/docker_publish.yml Show resolved Hide resolved
@giom-l giom-l requested a review from Haarolean June 21, 2024 22:15
@giom-l giom-l force-pushed the feat/add_dockerhub_publish branch from fb8c8bf to aa6a8e9 Compare June 21, 2024 22:49
@Haarolean Haarolean added the hacktoberfest-accepted PRs accepted towards hacktoberfest goal and will be counted as approved label Oct 4, 2024
@Haarolean Haarolean changed the title 🚀 Add Dockerhub publish during release Infra: Add actions to publish to ECR & Docker Hub Oct 7, 2024
@Haarolean Haarolean merged commit 07f0e0e into kafbat:main Oct 7, 2024
6 checks passed
@Haarolean
Copy link
Member

@giom-l thanks for your first (and a high quality one!) contribution to kafbat UI! I'll be taking a look at how this behaves now.

@Haarolean
Copy link
Member

@giom-l
Copy link
Contributor Author

giom-l commented Oct 11, 2024

It reminds me of something about provenance but can't remember exactly what ATM.

I'm pretty sure I tried multiple stuffs to get rid of that unsuccessfully.
I think that in the end, I preferred to keep provenance attestations with unknown arch over not having them at all.

I just found this discussion that seem related :
https://github.com/orgs/community/discussions/45969

But there does not seem to be proper answer (get rid of provenance, pinning version that seem to not work anymore...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted PRs accepted towards hacktoberfest goal and will be counted as approved scope/infra CI, CD, dev. env, etc. status/triage/completed Automatic triage completed type/enhancement En enhancement/improvement to an already existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infra: Upload images to a public AWS ECR Infra: Upload docker images to Docker Hub
2 participants