snok / container-retention-policy

GitHub action for pruning old GHCR container image versions.
MIT License
187 stars 30 forks source link

v3 release #84

Open sondrelg opened 4 months ago

sondrelg commented 4 months ago

If you notice any problems in the new release, please post them here πŸ‘

ubbeK commented 4 months ago

Will there be a v3 tag?

ubbeK commented 4 months ago

Will there be a v3 tag?

Sorry, didn't read the release notes too carefully

We'll no longer maintain mutable major and minor version tags for the action. There will be no v3 target for the action, just v3.0.0 and other exact versions.

sondrelg commented 4 months ago

Will there be a v3 tag?

Sorry, didn't read the release notes too carefully

We'll no longer maintain mutable major and minor version tags for the action. There will be no v3 target for the action, just v3.0.0 and other exact versions.

No worries ☺️ I could be convinced otherwise, but what we've done in the past is just tag every patch release with the equivalent of v3, which in practice means everyone's on latest, and means you break everyone if you introduce an issue, which is inevitable.

Dropping the mutable tags seems like it should:

If you're worried about falling behind on action versions, I can recommend using dependabot for github actions, like we do here πŸ‘ It will open a PR with the relevant release notes when there's a new action version available.

Let me know if this misses out on some other benefit of a v3 tag that I haven't considered!

ubbeK commented 4 months ago

Everybody does it differently regarding tags and mutable tags. I personally prefer mutable tags for major-tag and minor-tag (examples v3, v3.1) which gives the freedom for the end user to choose what kind of tag to use. As long as SemVer 2.0 is followed regarding the interface (input/output variable names and type/format), then I wouldn't effectively be targeting the main branch when using the major mutable tag. At the same time, I could target minor mutable tag to lock in on a specific state, but still get all patch fixes.

I am personally maintaining a few github actions and reusable github action workflows which are located in several private git repos, and I would like to share my workflow for PR merges which implements what I discussed above. I am not saying that you should implement it yourself, as I get that it is hard to test all scenarios for a small/medium sized opensource project.

on:
  pull_request: # on merge to main only, see if-statement: github.event.pull_request.merged == true
    branches:
    - main
    types:
    - closed

jobs:
  pr_merge:
    if: github.event.pull_request.merged == true
    runs-on: ubuntu-latest
    steps:
    - name: Checkout repo
      uses: actions/checkout@v4
      with:
        fetch-depth: 0 # due to paulhatch/semantic-version step below

    - name: Get next semver version
      id: get_version
      uses: paulhatch/semantic-version@v5.4.0
      with:
        tag_prefix: "v"
        version_format: "${major}.${minor}.${patch}"
        major_pattern: "(MAJOR)"
        minor_pattern: "(MINOR)"
        search_commit_body: true

    - name: Create release
      env:
        GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
      run: |
        gh release create "${{ steps.get_version.outputs.version_tag }}" \
          --repo="$GITHUB_REPOSITORY" \
          --title="${{ steps.get_version.outputs.version_tag }}" \
          --generate-notes

        git tag --force v${{ steps.get_version.outputs.major }}
        git push origin v${{ steps.get_version.outputs.major }} --force

        git tag --force v${{ steps.get_version.outputs.major }}.${{ steps.get_version.outputs.minor }}
        git push origin v${{ steps.get_version.outputs.major }}.${{ steps.get_version.outputs.minor }} --force

    - name: PR comment
      uses: actions/github-script@v7
      with:
        retries: 3
        script: |
          github.rest.issues.createComment({
            issue_number: context.issue.number,
            owner: context.repo.owner,
            repo: context.repo.repo,
            body: 'Release [${{ steps.get_version.outputs.version_tag }}](https://github.com/${{ github.repository }}/releases/tag/${{ steps.get_version.outputs.version_tag }}) created on SHA ${{ github.sha }}.\n\n' +
                  'Tag [v${{ steps.get_version.outputs.major }}](https://github.com/${{ github.repository }}/releases/tag/v${{ steps.get_version.outputs.major }}) created on SHA ${{ github.sha }}.\n\n' +
                  'Tag [v${{ steps.get_version.outputs.major }}.${{ steps.get_version.outputs.minor }}](https://github.com/${{ github.repository }}/releases/tag/v${{ steps.get_version.outputs.major }}.${{ steps.get_version.outputs.minor }}) created on SHA ${{ github.sha }}.'
          })

Keep up the good work, and I will definitely check out the dependabot example you supplied. Thanks!

raszi commented 4 months ago

I am having issues defining a rule that would remove every outdated image but the skipped ones. The previous config looked like this:

image-names: foo
cut-off: One month ago UTC
keep-at-least: 1
account-type: personal
skip-tags: base, production
token: ${{ secrets.GITHUB_TOKEN }}
token-type: github-token

The new:

account: user
token: ${{ secrets.GITHUB_TOKEN }}
image-names: foo
image-tags: *, !base, !production
cut-off: 4w
keep-n-most-recent: 1

But this does not work. GitHub says: Invalid workflow file because of invalid syntax on the image-tags line.

sondrelg commented 4 months ago

Please try image-tags: !base, !production or image-tags: !base !production @raszi πŸ‘ The wildcard is implicit as long as no positive matchers (expressions without ! prefixed) are specified.

I'll see if I can fix this in a patch release, so it would work in the future.

raszi commented 4 months ago

That does not work either. πŸ˜” Now it is complaining about the with: line. With or without commas, it has the same issue.

sondrelg commented 4 months ago

See this example which does the same thing.

Is your workflow public?

raszi commented 4 months ago

That example is not exactly the same, because that has an explicit wildcard. With the explicit wildcard, I get the error when I use *, as I pasted above. Without the explicit wildcard, I get the error on the with: level. If I use an explicit wildcard that looks like the example you linked (like t*), it has no errors, but that is not the same behavior.

No, unfortunately, the workflow is not public.

ubbeK commented 4 months ago

That example is not exactly the same, because that has an explicit wildcard. With the explicit wildcard, I get the error when I use *, as I pasted above. Without the explicit wildcard, I get the error on the with:. level. If I use an explicit wildcard that looks like the example you linked (like t*), it has no errors, but that is not the same behavior.

No, unfortunately, the workflow is not public.

I would guess that you need to make the yaml property it into a string to not get that with error, like

account: user
token: ${{ secrets.GITHUB_TOKEN }}
image-names: foo
image-tags: "!base !production"
cut-off: 4w
keep-n-most-recent: 1
raszi commented 4 months ago

I am unsure how the YAML value is handled by this project, but will that work the same way? Wouldn't it try to ignore images with tags base !production?

sondrelg commented 4 months ago

It should work the same way, but I'd suggest running the action with dry-run: true if you want to be sure. It'd be great if you could share the entire step definition, so it's possible to replicate the issue.

raszi commented 4 months ago

This is my current step definition:

  clean-old-images:
    name: Delete old unused images
    runs-on: ubuntu-latest

    steps:
      - name: Delete old images
        uses: snok/container-retention-policy@v3.0.0
        with:
          account: user
          token: ${{ secrets.GITHUB_TOKEN }}
          image-names: foo
          image-tags: "!base !production"
          cut-off: 4w
          keep-n-most-recent: 1
sondrelg commented 4 months ago

That should work @raszi πŸ‘ Quoting the values is fine. I see what you mean about it being illegal yaml. I'll update the docs and release note.

sondrelg commented 4 months ago

As a quick tip: you can run the action with rust-log: container_retention_policy=trace to get a more detailed look at what's going on. See the Delete old images step in https://github.com/snok/container-retention-policy/actions/runs/9664048758/job/26657757575 for an example

amoosbr commented 3 months ago

Hi, not sure if my question belongs to the v3 release or not. Just wanted to let you know, about my experience, when I tried to use v3.0.0 with a GitHub App token.

Based on the README image-names section, I expected to use a GitHub App for cleanup a image name like bla/* Readme snippet:

These operators are only available for personal- and GitHub app-tokens. See the token parameter section for more info.

Unfortunately, when I tried to use it as documented in the GitHub App token sample, I got the following response:

2024-07-29T09:34:18.768949Z DEBUG container_retention_policy: Logging initialized
2024-07-29T09:34:18.769670Z DEBUG parse input: container_retention_policy::cli::models: Recognized token as temporal token
2024-07-29T09:34:18.770018Z DEBUG parse input: container_retention_policy::client::builder: Constructing base urls
2024-07-29T09:34:18.770047Z DEBUG parse input: container_retention_policy::client::builder: Constructing HTTP headers
2024-07-29T09:34:18.770065Z DEBUG parse input: container_retention_policy::client::builder: Creating rate-limited services
2024-07-29T09:34:18.770328Z DEBUG fetch rate limit: container_retention_policy::client::client: Retrieving Github API rate limit
2024-07-29T09:34:18.842278Z DEBUG fetch rate limit: container_retention_policy::client::client: There are [15](https://github.com/ORG/actions/actions/runs/10141744949/job/28039764262#step:4:16)000 requests remaining in the rate limit
thread 'main' panicked at src/client/client.rs:46:17:
Restrictions in the Github API prevent us from listing packages when using a $GITHUB_TOKEN token. Because of this, filtering with '!' and '*' are not supported for this token type. Image name cache/* is therefore not valid.

Looking at the latest history, I saw a commit, that mentions GitHub App tokens behave like temporal tokens. I tried using building and running the 3.0.0 release candidate, where the GitHub App tokens was not treated as temporal token. Then my GitHub App token didn't have the needed scopes:

The token does not have the scopes needed. Tokens need `read:packages` and `delete:packages`. The scopes found were none.

My GitHub App has packages:write permission and is installed on the org with access to all repositories. I didn't find a packages:delete option for GitHub Apps.

I'm not 100% sure, if I just used the action wrong or my App has wrong settings. But if it is not possible to have wildcard image names with GitHub App tokens, perhaps the README can be updated.

As soon as I switched to using a classical PAT, the workflow stated to work as expected.

raszi commented 3 months ago

Quoting the values is fine.

Yes, this worked! Thanks!

sondrelg commented 3 months ago

Hi, not sure if my question belongs to the v3 release or not. Just wanted to let you know, about my experience, when I tried to use v3.0.0 with a GitHub App token.

Based on the README image-names section, I expected to use a GitHub App for cleanup a image name like bla/* Readme snippet:

These operators are only available for personal- and GitHub app-tokens. See the token parameter section for more info.

Unfortunately, when I tried to use it as documented in the GitHub App token sample, I got the following response:

2024-07-29T09:34:18.768949Z DEBUG container_retention_policy: Logging initialized
2024-07-29T09:34:18.769670Z DEBUG parse input: container_retention_policy::cli::models: Recognized token as temporal token
2024-07-29T09:34:18.770018Z DEBUG parse input: container_retention_policy::client::builder: Constructing base urls
2024-07-29T09:34:18.770047Z DEBUG parse input: container_retention_policy::client::builder: Constructing HTTP headers
2024-07-29T09:34:18.770065Z DEBUG parse input: container_retention_policy::client::builder: Creating rate-limited services
2024-07-29T09:34:18.770328Z DEBUG fetch rate limit: container_retention_policy::client::client: Retrieving Github API rate limit
2024-07-29T09:34:18.842278Z DEBUG fetch rate limit: container_retention_policy::client::client: There are [15](https://github.com/bosch-adas-genai/actions/actions/runs/10141744949/job/28039764262#step:4:16)000 requests remaining in the rate limit
thread 'main' panicked at src/client/client.rs:46:17:
Restrictions in the Github API prevent us from listing packages when using a $GITHUB_TOKEN token. Because of this, filtering with '!' and '*' are not supported for this token type. Image name cache/* is therefore not valid.

Looking at the latest history, I saw a commit, that mentions GitHub App tokens behave like temporal tokens. I tried using building and running the 3.0.0 release candidate, where the GitHub App tokens was not treated as temporal token. Then my GitHub App token didn't have the needed scopes:

The token does not have the scopes needed. Tokens need `read:packages` and `delete:packages`. The scopes found were none.

My GitHub App has packages:write permission and is installed on the org with access to all repositories. I didn't find a packages:delete option for GitHub Apps.

I'm not 100% sure, if I just used the action wrong or my App has wrong settings. But if it is not possible to have wildcard image names with GitHub App tokens, perhaps the README can be updated.

As soon as I switched to using a classical PAT, the workflow stated to work as expected.

Hmm, it sounds like we're assuming you're using $GITHUB_TOKEN when you're acually fine. Perhaps we should add a test to see whether requests like this will work or not, or find another way to differentiate the two token types.

As for the token permissions, I think these exist for PATs, but they may not do for app tokens? bilde

makkes commented 3 months ago

keep-at-least has apparently been removed in v3 but it's not documented in the migration guide.

sondrelg commented 3 months ago

keep-at-least has apparently been removed in v3 but it's not documented in the migration guide.

It was renamed to keep-n-most-recent: https://github.com/snok/container-retention-policy/blob/main/action.yaml#L30. Sorry, you're right, I missed that in the release note. I'll add it now πŸ‘