Byron / gitoxide

An idiomatic, lean, fast & safe pure Rust implementation of Git
Apache License 2.0
8.84k stars 301 forks source link

Allow keeping releases drafts via repo-level GHA variable #1499

Closed EliahKagan closed 1 month ago

EliahKagan commented 1 month ago

With this, the "Publish the release" step has an if: that will be true unless a GitHub Actions configuration variable named DRY_RUN_RELEASE--that is, a DRY_RUN_RELEASE in the vars context, not to be confused with an environment variable--is defined with one of the value that is often used in GHA workflows to express true conditions. The way this works is:

It seems to me that the added complexity is worthwhile to avoid having to add if: false while developing and experiment with the release workflow, to suppress the publication of a release, i.e., setting the draft release to non-draft:

The added complexity is only in the workflow itself. No burden is imposed on actual releasing, or on a repository whose maintainer elects not to add the variable:

I anticipate that, if you choose to merge this, then you would probably not set the variable here, and that, if you do, then you would probably set it to a value like false. However, this does not look at whether the repository is a fork, and its use is not limited to forks: if you merge this and want to test changes to the release workflow here, you can set the variable to one of the three recognized truthy values to keep the workflow from marking releases non-draft (which would affect all runs that start after the change).

Here's a workflow run that tests this, and here's the affected job.

Byron commented 1 month ago

That's great, thank you! I have set the variable in this repository to false for good measure.

And as always - what helps you does help me as well. Do feel free to add more as you discover new needs for it, or make other changes that make your work easier.

EliahKagan commented 1 month ago

And as always - what helps you does help me as well. Do feel free to add more as you discover new needs for it, or make other changes that make your work easier.

This is not related to releasing, but I would find it useful to run CI in feature branches in my fork without having to modify the workflow (i.e., using another branch with additional changes to make CI run) or open a PR.

I'm not sure if this should be done, though, since running it on both push and pull_request triggers in the upstream repository when a pull request is opened, and when commits are pushed to a branch in the upstream repository that has an open pull request, could be considered undesirable.

There may be ways to avoid that, but I would be concerned about the complexity of dong so. It might be achievable by introducing a new job that other test jobs declare a dependency on, that checks if the workflow is running on a push trigger from the same repository where the branch has an open PR. I'm not sure what the commit in the upstream repository would show for its CI status, though.

The opposite approach of cancelling pull_request triggered jobs when there is a corresponding push triggered job might work better in most cases, because it would prevent a duplicate job even for the commit that a PR is opened with. But unless it is only acting for jobs that run at the same time, it would have the problem that when a PR is opened much later such that changing conditions, e.g. with dependencies, would cause something to fail, this would not be discovered.

That caveat is also a reason not to take the approach of removing the pull_request trigger and leaving only the push trigger, though the bigger reason to avoid that is that contributions who open PRs may not have enabled CI in their forks.

Maybe the best thing to do is to run CI on the push trigger on main as is currently done and also on feature branches that are named according to a particular convention. This would lead to feature branch names that would be longer or less intuitive, but it should be straightforward to do, and it would not require any logic for checking what other runs exist, cancelling anything, or adding any new jobs.

Byron commented 1 month ago

I'm not sure if this should be done, though, since running it on both push and pull_request triggers in the upstream repository when a pull request is opened, and when commits are pushed to a branch in the upstream repository that has an open pull request, could be considered undesirable.

I suppose this would affect me as I currently push to branches in this repository that also have a PR usually, but I am more than happy to push into a fork instead. That way, this repository could be keep cleaner, I suppose many branches have accumulated by now.

Maybe the best thing to do is to run CI on the push trigger on main as is currently done and also on feature branches that are named according to a particular convention. This would lead to feature branch names that would be longer or less intuitive, but it should be straightforward to do, and it would not require any logic for checking what other runs exist, cancelling anything, or adding any new jobs.

That would also work, but like I said, I'd think it would be good to have an incentive not to push branches directly into this repository. If you were to submit a PR for this, I'd start working with a fork - it will be very welcome.

EliahKagan commented 1 month ago

How would you work with a fork? As I understand it, personal accounts on GitHub can't contain forks of their own repositories. Would you create an organization and have it fork the repository, then contribute from there? Or the reverse, creating an organization and moving this repository there, then creating the fork in your personal account (which would then have to have a name other than Byron/gitoxide, which would redirect to the repo in the org)?

Would you have CI turned off in your fork? If so, then you would be unable to attain the benefits that I seek in trying to enable the push trigger for feature branches. But if the workflow would be enabled in your fork, then I am not sure the change that we are discussing would actually provide an incentive not to push feature branches directly to this upstream repository. This would depend on how rate-limits are shared between repositories.

If a user creates an organization and runs CI both in an org-owned repository and in a personally owned repository, are those totally separate and independent caps? If not, then the push trigger in the fork and the pull_request trigger in the upstream repository would be slowing each other down (in the sense of making each other wait for available runners) in much the same way as when it is all done here.

Anyway, I would be happy to expand the push trigger to all branches, if that is something you want done. But some of the above may be reasons not to do so, or not to do so at this time, and may also touch on other changes that would have to be made (for example, if moving this upstream repository into a newly created GitHub organization, even though the original URL should work as a redirect, it should nonetheless be updated in a number of places). So I figured I would bring that stuff up before proceeding.

Byron commented 1 month ago

🤦‍♂️ I don't know what I was thinking - you are absolutely right - I of all the people can't have a fork without using another account of sorts.

With that clarified, maybe it's plan B then with glob that runs CI for feature branches/branches with a certain pattern. Maybe ci-run-for-branch/* would do, while clarifying what the purpose of the namespace is.

That shouldn't trigger for any of my branches then.

Is this something you have been proposing, or will this be another 🤦‍♂️ moment for me 😁?

EliahKagan commented 1 month ago

Is this something you have been proposing

Yes, this would be a case of the "named according to a particular convention" alternative (and the end of https://github.com/Byron/gitoxide/pull/1499#issuecomment-2278901849).

I'll see if I can think of a name like ci-run-for-branch that is shorter while still not causing confusion, though I suspect that might be the best name.

Assuming that name is used, then the patterns would probably be ci-run-for-branch/** and **/ci-run-for-branch/**, so it can be used together with other directory-like /-delimited prefixes. I'll want to check the specific globbing syntax the patterns use to make sure I understand the semantics correctly and also to see if it can elegantly be expressed with a single pattern.

EliahKagan commented 1 month ago

I have another thought: Would some of the considerations that had motivated you to want to contribute via a fork be mitigated by giving your branches distinctive names, such as with prefix like byron/ or b/ or something?

I only advocate this if that is already valuable in its own right. But if so, and if those are the ones that should not run on a push trigger, then perhaps, if it can be managed with the available syntax, it would be best to run CI checks for the push event on all branches except those.

The reason is that CI is turned off automatically in forks. Various occurrences relating to the introduction of new workflows enable it, but it is mainly enabled by deliberately doing so in the Actions tab. So any fork with CI turned on is probably a fork whose owner wants CI checks to run even separate from pull requests.

What do you think of this?

An alternative could be to do the opposite: any branch with / in its name would have CI checks run on push. Then I could name my branches with an ek/ or similar prefix.

Byron commented 1 month ago

A valid point!

I find myself creating single-word branches a lot though, but can probably train myself to use a prefix to exclude it from CI. But then I once again think it should be self-describing, like no-ci/*.

Feel free to submit a PR, let's just try it - mistakes I will definitely make 😁.

EliahKagan commented 1 month ago

But then I once again think it should be self-describing

I think this is good for troubleshooting whether and why CI ran, but actually bad otherwise, because it is likely to be confused with the content or purpose of the branch (i.e., with how one would ordinarily name the branch).

However, while I think we disagree about the desirability of the branch names being explicit:

So self-describing is the way to go.

Feel free to submit a PR, let's just try it

I've opened #1507, which seems to me may be consistent with both our goals and preferences.

like no-ci/*.

Naming is tough because, in my view, the primary purposes of a branch name is to distinguish the branch from other branches and to describe the content of the commits on the branch, and the primary purpose of a directory-like prefix is to group branches by what they are intended to do, rather than by how they are intended to be tested.

Nonetheless, a run-ci/ prefix or other directory-like component seemed intuitive to me, so I went with it in #1507. A prefix to not run CI seemed less intuitive to me, and I also didn't think of a good name that wouldn't be confused with the purpose of the branch. So you do not need to name your branches any differently.

In the specific case of no-ci/, I think that would produce confusing situations like branches named no-ci/more-ci or, worse, a no-ci/riscv branch whose specific purpose is to add CI tests for RISC-V but that is named that way because all the testing is being done in a draft PR.

To an extent this kind of thing could happen with any explicit naming convention, but in #1507 I went with run-ci instead of ci-run-for-branch because it is shorter and simpler, and because really both the push and pull_request triggers are running on a branch, so the greater detail didn't seem to carry with it a corresponding increase in clarity.