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

Add tag flavor #15

Closed
wants to merge 1 commit into from
Closed

Conversation

hugopeixoto
Copy link

@hugopeixoto hugopeixoto commented Nov 18, 2020

Fixes #13

For cases where you have multiple flavors of the same container, like node:12-alpine, node:12-debian, etc, it would be nice to be able to specify these flavors in this github action. This was requested in #13.

Let's say that your container has two flavors, debian and alpine, and that you want images like foo:latest to be based on the debian flavor. You might want to generate these tags:

  1. foo:x.y.z-debian
  2. foo:x.y.z-alpine
  3. foo:x.y.z (= foo.x.y.z-debian)
  4. foo:debian (= foo.x.y.z-debian)
  5. foo:alpine (= foo.x.y.z-alpine)
  6. foo:latest (= foo.x.y.z-debian)
  7. foo:sha-AAAAAA-debian
  8. foo:sha-AAAAAA-alpine
  9. foo:sha-AAAAAA (= foo:sha-AAAAAA-debian`)

To achieve this, I added two options: flavor (string) and main-flavor (boolean). Trying to summarize their behavior:

  • specifying a flavor will generate tags with the flavor suffix
  • if no flavor is specified, or if there is a flavor but main-flavor is true, the flavorless tags will be emitted as well
  • versionless flavorful tags (foo:debian) will only be generated if there's a flavor and the tag matches as latest.

In other words, each previously emited tag has its flavor counterpart:

  • foo:x.y.z => foo:x.y.z-{flavor}
  • foo:latest => foo:{flavor}
  • foo:sha-AAAAAA => foo:sha-AAAAAA-{flavor}

And the left side are emited if no flavor != '' or mainFlavor, and the right side is emited if flavor != ''.

This is how I'm using this in a project:

on:
  push:
    tags: '*.*.*'

jobs:
  release:
    strategy:
      matrix:
        base: [debian, alpine]
    steps:
      - uses:  hugopeixoto/ghaction-docker-meta@d08260d
        name: Docker meta
        id: docker_meta
        with:
          images: quay.io/hedgedoc/hedgedoc
          flavor: ${{ matrix.base }}
          main-flavor: ${{ matrix.base == 'debian' }}

I hope this is understandable :P Let me know what you think, and if this is an approach that makes sense. I wasn't sure if the sha tags should be flavored as well.

I still need to add tests and document this in the readme, but I'd like to know if this is something that you would consider integrate and iterate on before spending time there.

@codecov
Copy link

codecov bot commented Nov 18, 2020

Codecov Report

Merging #15 (94c31d2) into master (9de4428) will decrease coverage by 5.26%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #15      +/-   ##
==========================================
- Coverage   72.94%   67.67%   -5.27%     
==========================================
  Files           3        3              
  Lines          85       99      +14     
  Branches       18       27       +9     
==========================================
+ Hits           62       67       +5     
  Misses          6        6              
- Partials       17       26       +9     
Impacted Files Coverage Δ
src/context.ts 92.30% <ø> (ø)
src/meta.ts 67.53% <50.00%> (-7.08%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9de4428...94c31d2. Read the comment docs.

@crazy-max
Copy link
Member

@hugopeixoto Thanks for this PR. Can you merge please?

@hugopeixoto
Copy link
Author

@hugopeixoto Thanks for this PR. Can you merge please?

I've rebased the PR to the latest master, resolving the conflicts. Let me know if you meant something else.

@crazy-max
Copy link
Member

@hugopeixoto Ahah my bad was tired yesterday, I meant rebase 😅

@LongLiveCHIEF
Copy link

@hugopeixoto you beat me to it.

Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

Overall LGTM but need some changes:

  • Please don't create inline condition. We want to make sure code coverage is respected.
  • Add tests
  • Update README

action.yml Show resolved Hide resolved
src/context.ts Outdated Show resolved Hide resolved
src/meta.ts Outdated Show resolved Hide resolved
src/meta.ts Outdated Show resolved Hide resolved
src/meta.ts Outdated Show resolved Hide resolved
src/meta.ts Outdated Show resolved Hide resolved
src/meta.ts Outdated Show resolved Hide resolved
@yumetodo
Copy link

I want to append suffix(like -tesseract_opencv-4.5.0). When this PR is merged, can I do that?

@LongLiveCHIEF
Copy link

I suspect #24 may supersede this entirely, even though it may not have been intentionally designed to do so.

@crazy-max
Copy link
Member

@LongLiveCHIEF

I suspect #24 may supersede this entirely, even though it may not have been intentionally designed to do so.

Actually no, tag-custom add additional tags to those already produced and do not modify them as this PR does.

@hugopeixoto hugopeixoto force-pushed the feature/add-flavor branch 9 times, most recently from f64885a to 04d7e66 Compare December 1, 2020 22:23
@hugopeixoto
Copy link
Author

I've updated the PR to include documentation in README.md and some tests in meta.test.ts.

@crazy-max I'm fighting codecov and losing. The current problem seems to be the line let main = !flavor || this.inputs.mainFlavor;, which is marked as partial, but I think I have the four cases covered. Could you help me figure out what's missing? The coverage report from yarn test doesn't seem to be as extensive as whatever codecov does.

@yumetodo
Copy link

yumetodo commented Dec 2, 2020

How about this? Some coverage tools cannot recognize short circuits correctly.

    let tags: Array<string> = [];
    const tagPush = (tagStr: string) => {
      if (this.inputs.flavor) {
        tags.push(`${tagStr}-${this.inputs.flavor}`);
        if (this.inputs.mainFlavor) {
          tags.push(tagStr);
        }
      } else {
        tags.push(tagStr);
      }
    };
    for (const image of this.inputs.images) {
      const imageLc = image.toLowerCase();
      tagPush(`${imageLc}:${this.version.main}`);
      for (const partial of this.version.partial) {
        tagPush(`${imageLc}:${partial}`);
      }
      if (this.version.latest) {
        if (this.inputs.flavor) {
          tags.push(`${imageLc}:${this.inputs.flavor}`);
          if (this.inputs.mainFlavor) {
            tags.push(`${imageLc}:latest`);
          }
        } else {
          tags.push(`${imageLc}:latest`);
        }
      }
      if (this.context.sha && this.inputs.tagSha) {
        tagPush(`${imageLc}:sha-${this.context.sha.substr(0, 7)}`);
      }
    }

@hugopeixoto
Copy link
Author

How about this? Some coverage tools cannot recognize short circuits correctly.

Thanks for the suggestion. I tried a variation of that: instead of using let main = x || y, I tried this:

if (flavor === '') {
  main = true;
} else if (this.inputs.mainFlavor) {
  main = true;
} else {
  main = false;
}

And each of the main = true/false lines is green (hit), but the if/else lines are yellow (partial). It seems like every if statement in this file is being marked as partial:
https://codecov.io/gh/crazy-max/ghaction-docker-meta/pull/15/src/src/meta.ts

Could it be that there's a problem with codecov's typescript tooling? I rolled the commit back to !flavor || etc.

@yumetodo
Copy link

yumetodo commented Dec 2, 2020

Oh...

@crazy-max
Copy link
Member

crazy-max commented Dec 4, 2020

@hugopeixoto Yes don't worry about codecov for this kind of stuff, there's something wrong on their side or something needs to be configured on our side. Will check this out later.

@stale
Copy link

stale bot commented Jan 3, 2021

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

@stale stale bot added the 💀 stale label Jan 3, 2021
@yumetodo
Copy link

yumetodo commented Jan 4, 2021

@crazy-max Do you have any updates? Are there any other problems?

@stale stale bot removed the 💀 stale label Jan 4, 2021
@felbinger
Copy link

Exactly the feature I need 👍

@stale
Copy link

stale bot commented Feb 7, 2021

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

@lpil
Copy link

lpil commented Feb 14, 2021

Hi @crazy-max , is there more to do here? 🙏

@crazy-max
Copy link
Member

@lpil Will focus on proposal in #30 and also work to drastically reduce the workflow by removing a maximum of inputs. I keep this PR opens for now as it provides a good use case.

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.

Add appending option to tags
6 participants