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

Fix default branch reference in GitHub Actions workflow definitions #278

Merged
merged 4 commits into from
Oct 5, 2022

Conversation

japborst
Copy link
Member

@japborst japborst commented Oct 5, 2022

In #253 the GHA workflow was edited to use $default-branch. Whilst this works in workflow templates, this actually doesn't in a workflow.

Here, we revert to referencing master directly.

@japborst japborst added the chore A task not related to code (build, formatting, process, ...) label Oct 5, 2022
@japborst japborst requested review from Stephan202 and rickie October 5, 2022 06:25
Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

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

Nice catch! For others: see also this.

Suggested commit message:

Fix default branch reference in GitHub Actions workflow definitions (#278)

Comment on lines 5 to 6
branches:
- master
Copy link
Member

Choose a reason for hiding this comment

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

We could still do:

Suggested change
branches:
- master
branches: [ master ]

(Assuming the earlier intention was to match jdk: [ 11.0.16, 17.0.4 ] below. Or we could change both to multi-line lists. Maybe that's nicer?)

Copy link
Member

Choose a reason for hiding this comment

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

IMO the jdk field is a bit different as for matrix that is the way to define the values, see here.

AFAICS that is the only way there. For branches, especially if it is only one, I prefer the - master way.

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

The docs also use both approaches, not very consistent. [something] is the same as - something, so in the end shouldn't matter. It can be a shorthand, which are especially helpful for matrices, whereas branches are longer to begin with.

jdk: [ 11.0.16, 17.0.4 ]

instead of

jdk:
 - 11.0.16
 - 17.0.4

Nevertheless, I don't see us expanding this list to a wide range, so using the shorthand is pretty clean.

@Stephan202 Stephan202 added this to the 0.4.0 milestone Oct 5, 2022
Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

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

One answer/question.

Already approving as it is not a blocker.

Comment on lines 5 to 6
branches:
- master
Copy link
Member

Choose a reason for hiding this comment

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

IMO the jdk field is a bit different as for matrix that is the way to define the values, see here.

AFAICS that is the only way there. For branches, especially if it is only one, I prefer the - master way.

WDYT?

@rickie
Copy link
Member

rickie commented Oct 5, 2022

Okay clear, let's merge when the build is green 😄.

@japborst japborst changed the title Revert default branch Fix default branch reference in GitHub Actions definition Oct 5, 2022
@japborst japborst merged commit f4e191e into master Oct 5, 2022
@japborst japborst deleted the japborst/revert-default-branch branch October 5, 2022 10:48
@rickie rickie changed the title Fix default branch reference in GitHub Actions definition Fix default branch reference in GitHub Actions workflow definitions Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore A task not related to code (build, formatting, process, ...)
Development

Successfully merging this pull request may close these issues.

3 participants