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

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

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

EliahKagan
Copy link
Member

@EliahKagan EliahKagan commented Aug 8, 2024

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:

  • The step still does not run unless the preceding steps (and all jobs in the workflow, before them) succeeded, irrespective of whether, or to what value, the variable is set.
  • To minimize added complexity in the workflow, this does not strip whitespace, fold case, or diagnose weird values: anything besides true, yes, and 1 is treated as being falsy. (That is with respect to the value. Variable names themselves are automatically upcased when set.) This very deliberately includes the empty string, as obtained when the variable does not exist.
  • Unlike secrets, variables are not secret. But they are still not inherited by or otherwise shared with or among forks: each repository has its own value or absence thereof.

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 main reason to do that at all is that users can sometimes be notified about releases, even in forks.
  • My justification for the added complexity is not the convenience of being able to avoid this, which might not necessarily justify this, but instead to avoid the churn of adding and removing if: in commits that are conceptually about something else (or having additional commits just to do it). That is, I see the main benefit as making the history of the file easier to understand in the future. The convenience is also a benefit, though.

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:

  • The feature of being able to suppress setting a release non-draft through this GitHub Actions variable can be ignored completely, since the default is to run the step.
  • When making the change, I had worried it might issue a warning on the workflow about accessing a GHA variable that does not exist, but I tested this case and that does not happen. (I did not test it with debug logging enabled, but a diagnostic message about this does not seem like a problem to me if it happens when debug logging is enabled.)

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
Copy link
Member

Byron commented Aug 8, 2024

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.

@Byron Byron merged commit 321f5cd into GitoxideLabs:main Aug 8, 2024
19 checks passed
@EliahKagan EliahKagan deleted the dry-run branch August 8, 2024 16:34
@EliahKagan
Copy link
Member Author

EliahKagan commented Aug 10, 2024

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
Copy link
Member

Byron commented Aug 10, 2024

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
Copy link
Member Author

EliahKagan commented Aug 10, 2024

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
Copy link
Member

Byron commented Aug 10, 2024

🤦‍♂️
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
Copy link
Member Author

EliahKagan commented Aug 10, 2024

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 #1499 (comment)).

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
Copy link
Member Author

EliahKagan commented Aug 10, 2024

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
Copy link
Member

Byron commented Aug 10, 2024

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
Copy link
Member Author

EliahKagan commented Aug 10, 2024

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:

  • The naming convention itself should be easy to understand and predict, which is a strong argument in favor of an explicit convention. By this I am referring to the code of the workflow files being understandable.
  • Doing it any other way is more complicated and, to really fully automate the decision based on the conditions, more complicated in a way that is prone to leave cancelled jobs rather than never running them.
  • This should be done in a way that you are okay with. (I should also only do it in ways I am okay with, but I am okay with this way.)

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.

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

Successfully merging this pull request may close these issues.

2 participants