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

The suggested default branch example does not work for schedule #184

Closed
felipecrs opened this issue Apr 11, 2022 · 18 comments · Fixed by #192
Closed

The suggested default branch example does not work for schedule #184

felipecrs opened this issue Apr 11, 2022 · 18 comments · Fixed by #192

Comments

@felipecrs
Copy link
Contributor

felipecrs commented Apr 11, 2022

tags: |
  type=raw,value=latest,enable=${{ github.ref == format('refs/heads/{0}', github.event.repository.default_branch) }}

Here is an example:

https://github.com/felipecrs/jenkins-agent-dind/blob/master/.github/workflows/ci.yml#L35

https://github.com/felipecrs/jenkins-agent-dind/runs/5964649319?check_suite_focus=true#step:3:31

I would say that this variable does not get filled properly for schedule events, which is meaningful, as metadata-action relies on it.

felipecrs added a commit to felipecrs/docker-images that referenced this issue Apr 11, 2022
@felipecrs felipecrs changed the title The suggested _default branch_ example does not work for schedule The suggested default branch example does not work for schedule Apr 11, 2022
@felipecrs
Copy link
Contributor Author

While the best would be for GitHub to fix it, I wonder if we should do something to make users aware of it before recommending it, or to use another approach.

@felipecrs
Copy link
Contributor Author

@crazy-max
Copy link
Member

Can you add this step in your workflow and give me the output if possible please?: https://github.com/crazy-max/ghaction-dump-context#usage

@felipecrs
Copy link
Contributor Author

@felipecrs
Copy link
Contributor Author

felipecrs commented Apr 11, 2022

repository isn't set in such occasion.

@crazy-max
Copy link
Member

crazy-max commented Apr 11, 2022

Ok so looking at their docs, schedule event always runs on the default branch:

Scheduled workflows run on the latest commit on the default or base branch.

So I would say add this:

tags: |
  type=raw,value=latest,enable=${{ github.ref == format('refs/heads/{0}', github.event.repository.default_branch) || github.event_name == 'schedule' }}

But I think we could add a global expression named is_default_branch to ease the integration in your workflow:

tags: |
  type=raw,value=latest,enable={{is_default_branch}}

WDYT?

@felipecrs
Copy link
Contributor Author

felipecrs commented Apr 11, 2022

I think that the first approach would be fine, if we knew that schedule is the only outlier. I wonder what happens for other events, as there are many.

PS: I did it for my repository, as I know that I don't use anything but schedule, push and pull_request.

Having this said, I would conclude that this is not a reliable manner of obtaining the default branch at all (it's not even documented), and because of that, the second approach would be better indeed.

I would even add the is_default_branch to the list of outputs of the action, so that it can be also consumed in other parts of the workflow. As an useful helper.

@crazy-max
Copy link
Member

I think that the first approach would be fine, if we knew that schedule is the only outlier. I wonder what happens for other events, as there are many.

I have many fixtures too that covers most of them or at least most important imo: https://github.com/docker/metadata-action/tree/master/__tests__/fixtures

@crazy-max
Copy link
Member

crazy-max commented Apr 11, 2022

I would even add the is_default_branch to the list of outputs of the action, so that it can be also consumed in other parts of the workflow. As an useful helper.

This action is not a general purpose one for other steps and only exposes what it does. It should be done upstream by GitHub.

@felipecrs
Copy link
Contributor Author

Absolutely understood.

@crazy-max
Copy link
Member

@felipecrs You can test with crazy-max/docker-metadata-action@default-branch. See #192. Docs has also been updated: https://github.com/crazy-max/docker-metadata-action/tree/default-branch#global-expressions. Let me know if that sgty.

@felipecrs
Copy link
Contributor Author

@crazy-max that's awesome. I made a few suggestions in the PR btw.

@felipecrs
Copy link
Contributor Author

@crazy-max, I'm a little concerned about the new {{ is_default_branch }}. Previously, this:

tags: |
  type=raw,value=latest,enable=${{ github.ref == 'refs/heads/master' }}

Would only be enabled for push events to the master branch. But the new:

tags: |
  type=raw,value=latest,enable={{ is_default_branch }}

Not only gets enabled for push events on master branch, but also for pull_requests targeting the master branch. And no one wants to push the latest tag for a pull_requests.

@crazy-max
Copy link
Member

Not only gets enabled for push events on master branch, but also for pull_requests targeting the master branch. And no one wants to push the latest tag for a pull_requests.

Indeed, I will change that.

@crazy-max
Copy link
Member

There is also some events like schedule that always returns the default branch and a latest tag might not be what the user wants for this case. Wonder if we should then have some logic in our global exps like:

tags: |
  type=raw,value=latest,enable={{ is_default_branch && event == "push" }}

@felipecrs
Copy link
Contributor Author

There is also some events like schedule that always returns the default branch and a latest tag might not be what the user wants for this case.

In my case, that's exactly what I want. And I think it's reasonable, after all github.ref will be set to refs/heads/master for schedule events.

The reason why is_default_branch was introduced was to address the hardcoded branch name only, I think.

@felipecrs
Copy link
Contributor Author

felipecrs commented Apr 26, 2022

Having that said, it would be interesting to be able to mix global exprs with the workflow expressions for more granular control.

I just don't know if this is reason enough to bother introducing it (which I suppose will not be trivial).

@crazy-max
Copy link
Member

Yes let's keep it that way in the meantime. Will see if there is some traction from other users for this pattern.

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.

2 participants