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

Deploy additional Docker images to GitHub Container Registry #2117

Merged
merged 7 commits into from
Jan 10, 2023
Merged

Deploy additional Docker images to GitHub Container Registry #2117

merged 7 commits into from
Jan 10, 2023

Conversation

lars-reimann
Copy link
Contributor

@lars-reimann lars-reimann commented Dec 1, 2022

Fixes partially #1472

Proposed Changes

  1. Deploy additional Docker images to GitHub Container Registry. This would later allow action.yml to pull the MegaLinter image from GitHub instead of Docker Hub, which should speed up the pull phase of the action.

Readiness Checklist

Author/Contributor

  • Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
  • If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

@lars-reimann
Copy link
Contributor Author

lars-reimann commented Dec 1, 2022

Some questions about this:

  1. Should other deploy workflows also be updated? This includes
    • deploy-ALPHA-flavors.yml
    • deploy-BETA-flavors.yml
    • deploy-BETA-linters.yml
    • deploy-DEV.yml
    • deploy-DEV-linters.yml
    • deploy-RELEASE-linters.yml
  2. Should this PR also update action.yml or should this be done in a separate PR?
  3. How can I test these changes?

Once these questions are answered, I'll update the CHANGELOG accordingly.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 1, 2023

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this pull request should stay open, please remove the O: stale 🤖 label or comment on the pull request.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Jan 1, 2023
@bdovaz bdovaz removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Jan 1, 2023
@bdovaz
Copy link
Collaborator

bdovaz commented Jan 1, 2023

Ping @nvuillam, can you answer this questions?

@nvuillam
Copy link
Member

nvuillam commented Jan 1, 2023

@bdovaz beta, beta-flavors and beta-linters will validate that it work, so the release jobs have more chances to work too :)

@lars-reimann
Copy link
Contributor Author

lars-reimann commented Jan 1, 2023

I recently found this discussion claiming that GHCR is no faster than Docker Hub for GitHub Actions. So, I wonder whether it makes sense to further pursue this PR.

@nvuillam
Copy link
Member

nvuillam commented Jan 1, 2023

docker pulls on docker hub has download limitations (not applied on github actions), so it still can be iteresting to use ghcr image, especially for self hossted runners :)

@Kurt-von-Laven
Copy link
Collaborator

Kurt-von-Laven commented Jan 2, 2023

Thai Pangsakulyanont reported improved performance building and pushing to GHCR, which makes me skeptical that pulling is somehow slower. We wrote ScribeMD/docker-cache, automating approach 📦 in the blog post, and it outperforms pulling directly from Docker Hub. In any case, we should measure the performance ourselves since it may have changed over time.

@echoix
Copy link
Collaborator

echoix commented Jan 2, 2023

And what about simply when there are some outages? It is native to GitHub, and the whole build doesn't need to be done twice anyway, only two pushes instead of one.

@lars-reimann
Copy link
Contributor Author

I can continue with this next week. If someone else wants to work on it in the meantime, feel free to do so.

@lars-reimann lars-reimann changed the title Deploy release flavor images to GitHub Container Registry Deploy additional Docker images to GitHub Container Registry Jan 9, 2023
@lars-reimann lars-reimann marked this pull request as ready for review January 9, 2023 10:02
@lars-reimann lars-reimann requested a review from nvuillam as a code owner January 9, 2023 10:02
@codecov-commenter
Copy link

codecov-commenter commented Jan 9, 2023

Codecov Report

Merging #2117 (7ccbd12) into main (841b135) will increase coverage by 0.51%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2117      +/-   ##
==========================================
+ Coverage   82.60%   83.11%   +0.51%     
==========================================
  Files         170      170              
  Lines        4484     4484              
==========================================
+ Hits         3704     3727      +23     
+ Misses        780      757      -23     
Impacted Files Coverage Δ
megalinter/config.py 92.30% <0.00%> (ø)
megalinter/tests/test_megalinter/config_test.py 100.00% <0.00%> (ø)
megalinter/reporters/UpdatedSourcesReporter.py 89.74% <0.00%> (+2.56%) ⬆️
...alinter/tests/test_megalinter/helpers/utilstest.py 89.01% <0.00%> (+8.05%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bdovaz
Copy link
Collaborator

bdovaz commented Jan 9, 2023

@lars-reimann you need to add this word to .cspell.json file:

image

Copy link
Member

@nvuillam nvuillam left a comment

Choose a reason for hiding this comment

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

Great PR, nice job, thanks a lot for your contribution :)

@nvuillam
Copy link
Member

nvuillam commented Jan 9, 2023

When i try to merge i have an error :/

Screenshot_20230109-202615.png

@echoix
Copy link
Collaborator

echoix commented Jan 10, 2023

When i try to merge i have an error :/

Screenshot_20230109-202615.png

Did you try on web instead of app? I can't imagine why an OAuth app is involved in that. Of course I never saw the settings on admin side, but changes to workflows have been done even during the holidays without problems.

@Kurt-von-Laven
Copy link
Collaborator

By default, bots can't edit .github/workflows for security reasons (c.f., orgs/community#25222, orgs/community#26164). In this case, since the error is coming from an OAuth App, it's possible to grant it the workflow scope if desired. I don't know which OAuth App this is though, so I can't comment on whether granting it the ability to execute arbitrary code in our name would be wise.

@nvuillam nvuillam merged commit 1d74fa0 into oxsecurity:main Jan 10, 2023
@nvuillam
Copy link
Member

@Kurt-von-Laven @echoix it worked thru web UI :)

@nvuillam
Copy link
Member

thanks again @lars-reimann :)

@lars-reimann lars-reimann deleted the deploy_gcr branch January 10, 2023 12:08
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.

6 participants