Tweak GitHub Actions workflow files, do not restrict workflows to master branch only

[jajik]

This PR

• removes the limit on branches where the workflows can run (when applicable) so that people do not have to modify their forks' files (or work on master directly) in order to run actions before creating a pull request;

• merges the two almost identical workflow files for CodeQL workflows into a single file using the included matrix. Removes some boilerplate from the file and incorrect comment.

(Given the changes are trivial and related to GH tooling only I did not create a JIRA ticket.)

[jajik]

(just a rebase to be up to date)

[garydgregory]

@jajik
Please see my comments.

[garydgregory]

Why are push branches removed in several files?

In Commons IO for example (https://github.com/apache/commons-io/blob/master/.github/workflows/codeql-analysis.yml), we specify master and these actions run for pull requests.

[jajik]

Because this limits the CI execution to the master branch only.

For this repository, the behavior remains the same as the master is the only branch that is used.

For forks this allows execution outside of master, so for example I opened this PR from a different branch than master. With branches: [ master ] present I won't see a pipeline run unless I create a PR against master (and then it requires an approval). When you remove it (as this PR does), the pipeline will run in all branches in forks on push, so that people can see results before creating a PR.

It's a big quality of life improvement, because it does not require people work in their master branches, nor modifying workflow files in their forks in order to run CI.

[garydgregory]

If you want to run CI in this repository, you need approval from one of its members.

[jajik]

For example you can see that there's a pipeline running for Windows workflow in my ci branch here

https://github.com/jajik/commons-daemon/actions/workflows/windows.yml?query=branch%3Aci

but if you create a custom branch in your fork and push into it, there won't be any.

[jajik]

If you want to run CI in this repository, you need approval from one of its members.

I know, I'm talking about actions run within the forks.

Let's take this PR as an example. There was no pipeline run because I did not get an approval. So without removing the branches: [ master ] I wouldn't know whether my pull request / my changes did not break anything. But when we remove this branches: [ master ] limitation, a pipeline will run in my fork under my account at the moment I push anything into any of my branches. So I can then have at least partial certitude that I did not break anything.

Is it clear what I mean?

[jajik]

If the branch restriction is removed, every PR to commons-daemon will cause the workflow to run twice:

1. On push — triggered when I push the branch to the repository.

2. On pull_request — triggered again when the PR is opened.

Well, yes and no. The 1. will run before a PR is opened in the repository where the branch is. That may be a feature branch in commons-daemon but it may be a fork as well. I would argue that it's better to know whether something is broken before creating a PR (that will always run in commons-daemon).

I understand the need to test workflow changes more flexibly, but there are alternatives that avoid this duplication:

• Temporarily remove the branch restriction in your personal fork while working on the workflow.

• Restore the restriction before opening the PR to apache/commons-daemon.

This way, you can still test changes freely without doubling the CI runs on every PR from committers.

That's not that easy. To remove the branch restriction it's necessary to modify the workflow file which means adding a new commit atop. When the upstream adds a new commit, this workflow modifying patch must be discarded, the new commits from the upstream consumed and then the discarded commit must be added again. Then you can rebase a feature branch from which you must omit the workflow patch. It's definitely doable, but imho it's not very friendly and is unnecessary.

The question is why is the "duplication" a problem here?

[ppkarwasz]

An example illustrates the issue best: take a look at the workflow runs for each commit in apache/commons-compress#685. Every time a commit was pushed to address a review comment, the workflows ran twice, once for the push event and again for the pull_request event, doubling the CI load unnecessarily.

As I mentioned above, I understand the frustration of testing PRs that modify workflows: it’s challenging both to prepare and to review, since the effect is not visible until after the PR is merged. However, such workflow-changing PRs are the exception, not the norm.

[jajik]

An example illustrates the issue best: take a look at the workflow runs for each commit in apache/commons-compress#685. Every time a commit was pushed to address a review comment, the workflows ran twice, once for the push event and again for the pull_request event, doubling the CI load unnecessarily.

I understand what you mean, but

1. The CI load in the project is "doubled" because of using branches (in the project) instead of forks.

2. I don't see what's the issue/downside with the "duplication" in case of using branches.

3. Using branches is restricted to maintainers/committers only (?), and yet most of the PRs are from forks anyway. (To be honest, dependabot has a lot of PRs too, but we can ignore its branches for the push trigger.)

As I mentioned above, I understand the frustration of testing PRs that modify workflows: it’s challenging both to prepare and to review, since the effect is not visible until after the PR is merged. However, such workflow-changing PRs are the exception, not the norm.

But the issue at hand is not limited to workflow changing PRs, quite contrary. It's about testing of all changes before a PR is created and its workflow is approved to run in this repository

[jajik]

(To be honest, dependabot has a lot of PRs too, but we can ignore its branches for the push trigger.)

Added in 5fab4e0 using branches-ignore.

[jajik]

@ppkarwasz Could you please have a look at my latest comments? I believe there was a misunderstanding about the motivation behind removing the limitation.

Also note, that the duplication with the feature branches can be handled by the branches-ignore I added for the depenabot.

[jajik]

@jajik Please see my comments.

@garydgregory Thank you for the review! I tried to make myself clearer (and fixed the non-removed file).

[jajik]

(again just a rebase)

[jajik]

(rebase again)

[github-advanced-security]

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

[ppkarwasz]

BTW: please don't force-push changes to this PR, since it prevents from reviewing it incrementally and see what changed in each new commit.

[ppkarwasz]

LGTM, thanks! 💯

@garydgregory, do you have any unsolved remarks or should I merge this?

[jajik]

@garydgregory Please, can you review whether we did not miss anything so that we can merge it eventually? Thank you.

[ppkarwasz]

@jajik,

While we wait for Gary's review, can you merge master into this PR and resolve the conflicts?

[jajik]

@jajik,

While we wait for Gary's review, can you merge master into this PR and resolve the conflicts?

Done.

[jajik]

(just another rebase)