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

Proposal: Redesign tagging input/outputs to be independent #30

Closed
LongLiveCHIEF opened this issue Dec 5, 2020 · 13 comments · Fixed by #50
Closed

Proposal: Redesign tagging input/outputs to be independent #30

LongLiveCHIEF opened this issue Dec 5, 2020 · 13 comments · Fixed by #50

Comments

@LongLiveCHIEF
Copy link

LongLiveCHIEF commented Dec 5, 2020

I still use this action for labeling features, but if you're open to it, I'd really like you to consider importing my actions tag handling capabilities due to their power, simplicity, and flexibility.

To give you an example, here's a snippet from a repo where I'm using the latest functionality I've implemented, snippet pulled from https://github.com/OctoPrint/octoprint-docker/blob/master/.github/workflows/octoprint-release.yml#L69-L87:

      -
        id: buildxy
        run: echo '::set-output name=buildxy::${{ (github.event.action == 'deploy'|| github.event.action == 'released') && github.event.client_payload.tag_name == steps.stable-octoprint.outputs.release }}'
      -
        name: Tagging Strategy
        id: tagging
        uses: HackerHappyHour/tagging-strategy@v3
        with:
          image_name: octoprint/octoprint
          tag_name: ${{ github.event.client_payload.tag_name }}
          tags: |
            %X%::${{ steps.buildxy.outputs.buildxy }}
            %X.Y%::${{ steps.buildxy.outputs.buildxy }}
            %X.Y.Z%
          extra_tags: |
            latest::${{ steps.stable-octoprint.outputs.release == github.event.client_payload.tag_name }}
            edge::${{ github.event.action == 'prereleased' || github.event.client_payload.tag_name == steps.latest-octoprint.outputs.release }}
            canary::${{ github.event.action == 'canary' }}
            bleeding::${{ github.event.action == 'bleeding' }}

This one snippet demonstrates the how i think the approach to tagging should work in ghaction-dockermeta:

  • explicit control over the conditions of each and every tag,
  • semver parsing
  • no interdependent input parameters,
  • output compatible with docker/build-push-action
  • supports tag suffixes Add tag flavor #15
  • supports additional non-semver custom tags Allow to add custom tags #24 via extra_tags
  • compatible with externally triggered events
  • sha tags (via extra_tags parameter in a similar way to your {{raw}} usage

Bonus

As far as I can tell, refactoring tag inputs to work as they do in HackerHappyHour/tagging-strategy@v3+ would close the following open issues as well:

I doubt my action will ever get any users (even though I obviously have a super high opinion of it 😏 ), but you've got the benefit of having been in the docker action space since actions launched, and the have visibility and reach. I'd love for the work I did to benefit your users and carry over here.

Would you be willing to consider a redesign to bring support for this usage technique? I'd really love to be able to archive my project and merge it with yours!

@LongLiveCHIEF
Copy link
Author

@crazy-max what are your thoughts on this?

@crazy-max
Copy link
Member

@LongLiveCHIEF Sorry for the delay! I like the idea of conditional assignment. I will make a quick review of the current implementation and see if this can fit this project. Keep you in touch.

@mblaschke
Copy link

Any progress on this? I'm interested in prefix/suffixes as this is would make it possible to use multiple Dockerfiles in one project.

@LongLiveCHIEF
Copy link
Author

LongLiveCHIEF commented Feb 13, 2021

@mblaschke this is fully implemented in https://github.com/marketplace/actions/tagging-strategy, and you could always use that in the meantime until @crazy-max makes a final decision.

It doesn't do label generation, so you'd still need this action if you want labels.

If you want to see some examples of a robust implementation, check out https://github.com/OctoPrint/octoprint-docker/blob/master/.github/workflows/octoprint-release.yml#L160-L174

... there are several examples of all the features in the github.com/OctoPrint/octoprint-docker github actions.

Edit, the tag name passed into it comes from an event payload from the OctoPrint/Octoprint workflow: https://github.com/OctoPrint/OctoPrint/blob/master/.github/workflows/trigger_docker.yml

@crazy-max
Copy link
Member

crazy-max commented Mar 20, 2021

@LongLiveCHIEF I have started to work on this yesterday. Here is a draft of what I plan to put in place for a v2:

New:

  • tags
  • labels

Kept:

  • images
  • sep-tags
  • sep-labels

Removed:

  • tag-sha
  • tag-edge
  • tag-edge-branch
  • tag-semver
  • tag-match
  • tag-match-group
  • tag-latest
  • tag-schedule
  • tag-custom
  • tag-custom-only
  • label-custom

Migration

tag-sha

tags: |
  type=sha,enable=true,latest=false,prefix=sha-,suffix=

Defaults:

  • enable=false
  • latest=false
  • prefix=sha-
  • suffix=

tag-edge / tag-edge-branch

tags: |
  type=edge,enable=true,latest=false,branch=${default_branch}

Defaults:

  • enable=false
  • latest=false
  • branch=${default_branch}

tag-semver

tags: |
  type=semver,enable=true,latest=true,pattern=,prefix=,suffix=

Defaults:

  • enable=true
  • latest=true
  • pattern=
  • prefix=
  • suffix=

tag-match / tag-match-group

tags: |
  type=match,enable=true,latest=true,group=,prefix=,suffix=

Defaults:

  • enable=false
  • latest=true
  • group=
  • prefix=
  • suffix=

tag-latest

Will be available for all types through latest=true|false keypair.

tag-schedule

tags: |
  type=schedule,enable=true,latest=false,pattern=,prefix=,suffix=

Defaults:

  • enable=false
  • latest=false
  • pattern=
  • prefix=
  • suffix=

tag-custom / tag-custom-only

tags: |
  type=tag,enable=true,latest=false,acustomtag
# or
tags: |
  acustomtag

Defaults:

  • enable=false
  • latest=false

labels

labels: |
  maintainer=CrazyMax

Flavor

We could also add a new flavors input to handle suffix/prefix/... for all tags:

Prefix and suffix all tags

flavors: |
  type=prefix,foo-
  type=suffix,-bar

Prefix and suffix a specific tag

tags: |
  type=sha,enable=true,latest=false,prefix=foo-,suffix=-bar

Optional keys

enable, latest, prefix, suffix inputs are optional.

@crazy-max
Copy link
Member

crazy-max commented Mar 20, 2021

I also thought about removing the images input and replace it like this:

tags: |
  type=image,username/app,ghcr.io/user-name/app

As everything would be now in the tags input we could also move everything directly in buildx UI to handle that natively with our build-push-action. Have to talk with @tonistiigi about that.

@LongLiveCHIEF
Copy link
Author

LongLiveCHIEF commented Mar 20, 2021

You've given this a lot of thought. I like how you took what had done and have made it even more powerful (which is what I was hoping for!)

What I can't tell is where the tag itself fits into the tags input? I'm also assuming the enable is what allows you to control whether or not each tag in the list is produced dynamically at build time? (in the way I was using ::true or ::false at the end of a list entry so you could specify if that tag was built?

@LongLiveCHIEF
Copy link
Author

To make sure I'm reading your implementation details correctly, i took my original example, and re-did it using your proposed implementation. Let me know if it looks right?

tags: |
    type=tag,latest=true,enable=${{ steps.stable-octoprint.outputs.release == github.event.client_payload.tag_name }}
    type=tag,enable=${{steps.buildxy.outputs.buildxy}},pattern={{major}}
    type=tag,enable=${{steps.buildxy.outputs.buildxy}},pattern={{major}}.{{minor}}
    type=tag,pattern={{version}}
    type=tag,enable=${{ github.event.action == 'prereleased' || github.event.client_payload.tag_name == steps.latest-octoprint.outputs.release }},edge
    type=tag,enable=${{ github.event.action == 'canary' }},canary
    type=tag,enable=${{ github.event.action == 'canary' }},bleeding

@LongLiveCHIEF
Copy link
Author

I really, really like this, assuming I'm reading it correctly. Let me know how I can help.

@LongLiveCHIEF
Copy link
Author

One thing I'm not sure I've seen yet, is this requirement:

  • compatible with externally triggered events

In my action, i have a tag_name input parameter that is used as a way to specify the tag from an external payload. I think we need a type=external option.

@LongLiveCHIEF
Copy link
Author

LongLiveCHIEF commented Mar 20, 2021

or maybe:

New Feature: Manual/Externally controlled version tags

tags: |
  type=external,tag=${{ github.event.client_payload.tag_name}}

Defaults:

  • enable=true
  • latest=false

Optional Keys

enable, latest, prefix, suffix, tag inputs are optional.

@LongLiveCHIEF
Copy link
Author

Actually, I propose we get rid of type= entirely in support of just tag or input or something like that.

Description

I think the thing that's missing is that the basic implication of this action is that the Dockerfile is building the project from within that project. This works well if you're building the official image of a project. Say my project is foo, and the image i'm publishing is foo:<tag>.

However, the other major use case that this action has been unable to be used for is when a community wants to build their own version of an image. For example, let's say my organization is called bar, and I want to build the foo project in my own image with some customizations, the image would be bar/foo:<tag>. I would have a runner watching for releases of the upstream foo incident, and I would trigger my bar/foo:<tag> workflow based on release/tagging/branching/other events from the upstream foo project.

However, I think supporting this ability would actually make things easier for the maintainers and users of this action.

Proposal

Here are the type values I've gleaned from your spec so far:

  • tag
  • schedule
  • match
  • edge
  • semver

What I'm thinking, is that the default behavior for each list item is what you are currently envisioning for type=semver, and for everything else, there will be input=<value or expression>. If no input is specified, then it will look to assign a default input using a release/tag event from the repository in context.

for schedule the tag parameter would be:

tags: |
  tag=$(date + '%Y-%m-%d')

(This uses the core date function that should be in every runner)

for match and edge... couldn't this be removed entirely, since you can provide tag=<expression output> or customtag anyways?

for type=semver... this behavior is tied to pattern= already anyways, and seems redundant, and therefore shouldn't be needed. If the value of pattern is a valid semver pattern, it will try to parse the value resolved to tag. If the value resolved in tag is not a valid semver value, then it should error.

Complete Example

The example below is each type of tag, but eliminates type= entirely, and uses tag instead.

steps:
  - id: match
    // some step (action or run command) that parses some input against desired regex and outputs desired tag to use
  - id: date
    run: echo "::set-output name=date::$(date +'%Y-%m-%d')"
tags: |
  pattern={{version}} #outputs foo/foo:X.Y.Z
  tag=${{ steps.date.outputs.date }},prefix=foo-                                                    #outputs foo/foo:foo-YYY-mm-dd
  tag=edge,enable=${{ some.user.specified.condition == conditions.for.edge }}  #will create foo/foo:edge if conditions match
  tag=${{steps.match.outputs.tag}}                                                                        #will create foo/foo:<whatever steps.match.outputs.tag resolves to>
  tag=${{ github.sha }}                                                                                            #will create foo/foo:<sha-hash-of-commit-that-triggered-workflow>
  tag=<input from arbitrary payload>,pattern={{major}},prefix=foo                        #will create whatever arbitrary input your expression resolves to and treat it as a semver tag due to non-null value of `pattern`, with result being foo/foo:foo-<major>

@crazy-max crazy-max mentioned this issue Mar 24, 2021
Merged
@crazy-max
Copy link
Member

@LongLiveCHIEF Ok I changed things a bit. I have opened a PR #50 still WIP where we can continue to discuss it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants