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

Use docker/metadata-action for deploy-DEV.yml #3193

Merged
merged 10 commits into from
Dec 28, 2023

Conversation

echoix
Copy link
Collaborator

@echoix echoix commented Dec 6, 2023

Since the workflow of DEV-linters worked correctly in the last couple of weeks, including with dependabot PRs, I applied a similar patch for the DEV workflow. This will allow all checks to be run for dependabot PRs. At the moment, these changes do not touch images that would be pushed to registry.

This is a reduced version of my experiments of yesterday, where I managed to add more advanced metadata to the manifests, had OCI manifest images instead of docker manifest images while still having them used in the same workflow (ie: what was blocking us of building and loading multiplatform images @bdovaz !!), and even was able to use some advanced exporters, like eStargz and zstd. (Only zstd managed to be a little faster at the end of the build, but had to pull later on.) However I’m not finished exploring it since I hit some no more space after the tests, probably since I had two copies of the image. I was using a service container for the job, with a local registry (at localhost:5000), that we can push to without it being on docker hub or other.

Some nice things might be close ;)

Proposed Changes

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

@echoix
Copy link
Collaborator Author

echoix commented Dec 6, 2023

I’m thinking about when our images would change of manifest type to be OCI images, and potentially lazy-loadable. That would mean it would be a v8 release? Would it be too much to do in like 2-3 months? Would we have other bigger changes that we would like to have?

@nvuillam
Copy link
Member

nvuillam commented Dec 6, 2023

@echoix as I'm a docker fraud (like on many on the topics) I don't understand in details everything you are saying, but it seems convincing, and you have my full trust, so if it ensures ascending compatibility to current MegaLinter users, I'm all in :)

For a v8, usually it requires time, if/when we succeed to have multiplatform builds, I'm more than ok to take some time to release it :)

@echoix
Copy link
Collaborator Author

echoix commented Dec 6, 2023

This PR does nothing to change externally (that's why I didn't apply yet in this step to beta and release workflows). And also why I didn't wait to have completely finished with my experiments, at least having the CI work for other auto updates means I'm able to maintain it during the week, if it only requires analyzing and merging. Coding takes a bit more time.

@echoix
Copy link
Collaborator Author

echoix commented Dec 6, 2023

I think we could easily progressively switch all of our workflows to metadata actions while in v7, and then add the changes that make it a little bit more different in v8. This means to not use sbom: true, provenance: true, provenance: mode=max or annotations: ${{ steps.meta.outputs.annotations }} in the docker/build-push-action (as it requires creates a manifest index, instead of just one image, the same that enables to have multiplatform images).

@echoix echoix marked this pull request as draft December 6, 2023 23:03
@echoix
Copy link
Collaborator Author

echoix commented Dec 6, 2023

Converting to draft since the same actions ran in my fork had an unexpected failure and I want to take a look before

@@ -259,7 +234,7 @@ jobs:
run: cd mega-linter-runner && sudo yarn install --frozen-lockfile && sudo npm link
- name: Run mega-linter-runner tests
if: ${{ steps.docker_build.outcome }} == 'success' && !contains(github.event.head_commit.message, 'quick build')
run: cd mega-linter-runner && MEGALINTER_RELEASE=${{ steps.image_tag.outputs.tag }} MEGALINTER_NO_DOCKER_PULL=true npm run test
run: cd mega-linter-runner && MEGALINTER_RELEASE=${{ fromJSON(steps.meta.outputs.json).labels['org.opencontainers.image.version'] }} MEGALINTER_NO_DOCKER_PULL=true npm run test
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nvuillam is there a way here to have it use the full image name if I give it the image name like echoix/megalinter:docker-metadata-action-only (or someday the full image name like localhost:5000/echoix/megalinter:docker-metadata-action-only, ghcr.io/echoix/megalinter:docker-metadata-action-only or docker.io/echoix/megalinter:docker-metadata-action-only)

Before, the env variable contained only the tag, and thus I changed it to be equivalent, but it only works for our repo, not for any forks, since it uses oxsecurity/megalinter unconditionally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixing this and the PR would be good to go (with a changelog entry) would there be better ways to not having to go back and manually write a changelog entry once we know the PR number? It's the single thing that chills me off of doing a PR to fix something.

@echoix
Copy link
Collaborator Author

echoix commented Dec 13, 2023

Ok, what I’d want:
Something similar to:

const release = process.env.MEGALINTER_RELEASE || "beta";

But for the image option.

{
option: "image",
alias: "d",
type: "String",
description: "MegaLinter docker image",
example: [
"ghcr.io/oxsecurity/megalinter:latest",
`ghcr.io/oxsecurity/megalinter:${DEFAULT_RELEASE}`,
`my-registry.com/mega-linter-python:${DEFAULT_RELEASE}`,
],
},

Is it possible to pass other args to the command without changing the test code? Or is it really needed to add a env var flag for that?

@echoix
Copy link
Collaborator Author

echoix commented Dec 15, 2023

I think I managed to do something that could work. Please wait for me to check that the same workflows pass in my repo too to make sure that it works properly (as it should work here, but not necessarily elsewhere)

@echoix
Copy link
Collaborator Author

echoix commented Dec 15, 2023

It « works » as is doesn’t fail, but if you look at this step (of the mega linter runner tests (npm run test)), the at the very top, it pulls the docker image for oxsecurity/megalinter:beta, but I think I added the --image argument everywhere I think it should have been used.

https://github.com/echoix/megalinter/actions/runs/7217127730/job/19664442064#step:18:33

Did I miss something? Which test step calls that? I hope it doesn’t need to pull the image only to display help.

And I think I realized that maybe that was the issue that had my experiment of using a local registry end up with no space left: It might had pulled an other image at this step, thus having 2 (or more) copies of Megalinter on disk, running out of space

@nvuillam
Copy link
Member

I see you use MEGALINTER_IMAGE env var, but when is it supposed to be set in the workflows ?

@echoix
Copy link
Collaborator Author

echoix commented Dec 16, 2023

What I'm not sure, is are the other tests above run before, and do they need to pull the image in order to display help, or upgrade the config files? It shouldn't

@nvuillam
Copy link
Member

@echoix so it is ok to merge ? if u are around maybe u can update the changelog.md ? :)

@echoix
Copy link
Collaborator Author

echoix commented Dec 16, 2023

Well, I still need help to understand why a pull of the beta image, not the one just build, is done for some of the tests in npm test, as it is unexpected. And will need to be fixed before I can do another step with this line of changes.

@echoix
Copy link
Collaborator Author

echoix commented Dec 18, 2023

Once #3233 is merged, this PR could be updated (or not), and directly be merged. Its ready.

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.

It seems to give simpler workflows, let's give it a try ;)

@nvuillam nvuillam merged commit fc58836 into oxsecurity:main Dec 28, 2023
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants