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 GitHub workflow_dispatch events to publish #7462

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

spydon
Copy link

@spydon spydon commented Feb 5, 2024

This PR allows new versions of packages to be published from GitHub workflow_dispatch events.
The same restriction on the refType is in place, it still needs to be run targeting a tag.

Closes: #7177

Reason for change:
This is currently a blocker for actions like the melos-action to be used to its full potential, where the user can create releases directly from the GitHub UI in the following manner:

  1. The maintainer presses "Run workflow".
    image
  2. The action creates a branch with updated CHANGELOG.md and pubspec.yaml files and uploads it as a PR
  3. A maintainer reviews and merges the release PR
  4. Another workflow is triggered by the merge that then tags the packages and triggers yet another workflow through workflow_dispatch, since tags created from a GitHub action aren't allowed to trigger other actions
  5. The last workflow uploads the newly versioned packages to pub.dev

This is a lot less error prone than doing manual releases from the command line, especially in bigger monorepos.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@jonasfj
Copy link
Member

jonasfj commented Feb 8, 2024

Are you sure these checks will still work?

    if (agent.payload.refType != 'tag') {
      throw AuthorizationException.githubActionIssue(
          'publishing is only allowed from "tag" refType, this token has "${agent.payload.refType}" refType');
    }
    final expectedRefStart = 'refs/tags/';
    if (!agent.payload.ref.startsWith(expectedRefStart)) {
      throw AuthorizationException.githubActionIssue(
          'publishing is only allowed from "refs/tags/*" ref, this token has "${agent.payload.ref}" ref');
    }
    final expectedTagValue = tagPattern.replaceFirst('{{version}}', newVersion);
    if (agent.payload.ref != 'refs/tags/$expectedTagValue') {

Will it require that the workflow_dispatch is triggered on a tag?

@spydon
Copy link
Author

spydon commented Feb 8, 2024

Will it require that the workflow_dispatch is triggered on a tag?

It will indeed, I'll add another test for that, ensuring that it gives an error message if it's not triggered on a tag.

@spydon
Copy link
Author

spydon commented Feb 8, 2024

@jonasfj I added a test in 1be34bf

I'm not sure if it is possible to trigger the second if, possibly by manipulating the request somehow, because if the refType is tag the ref must surely start with refs/tags right? Anyhow, if anyone would manage to do that it is still covered, just like it was before.

app/lib/package/backend.dart Outdated Show resolved Hide resolved
@Gustl22
Copy link

Gustl22 commented Mar 24, 2024

@spydon seems, there are some conflicts 😸 I'm excited about this feature ;D

@spydon
Copy link
Author

spydon commented Mar 24, 2024

@spydon seems, there are some conflicts 😸 I'm excited about this feature ;D

It's just the changelog, it has to be updated every time something else is merged (gives me throwbacks from before we used Melos in all our projects 😅).

@Gustl22
Copy link

Gustl22 commented Mar 29, 2024

@clragon @jonasfj anything blocking to finish the PR? Best regards :)

@spydon
Copy link
Author

spydon commented Mar 29, 2024

@clragon @jonasfj anything blocking to finish the PR? Best regards :)

The design document needs to be reviewed and accepted first, can be seen here: #7177 (comment)

And hopefully after that there only needs to be a pair of check boxes added and theeen we can get this in. 😄

@isoos
Copy link
Collaborator

isoos commented May 14, 2024

@spydon: We've come to a conclusion and we'd like to allow this event, but only as a UI option after an admin allowed it on the admin UI. Would you be interested to implement it as such?

It would need to be stored on the GithubPublishingConfig object, and be displayed on the UI of the package page admin.

@jonasfj: I think we could use a simple bool? isWorkflowDispatchEnabled flag on the object and a checkbox on the UI - or would you like to see a more open text field for the additional event names?

@jonasfj
Copy link
Member

jonasfj commented May 14, 2024

@isoos I think we should do two booleans.

bool isFromWorkflowDispatchEventEnabled;
bool isFromPushEventEnabled;

Or maybe allowPublishFrom<Event>, but we probably need to make two checkboxes in the UI:

  • Enable publishing from push events.
  • Enable publishing from workflow_dispatch events.

Where the first defaults to being enabled, and the second can be enabled.

I think the ability to disallow publishing from push events is nice. I think we can allow the freedom of letting people enable both, but let's give them the ability to only allow the thing they use.

@jonasfj
Copy link
Member

jonasfj commented May 14, 2024

@isoos this is why I thought it might be necessary with some data migration, in order to just create the properties on all entities. That's something we could easily do with a backfill job, right?

@isoos
Copy link
Collaborator

isoos commented May 14, 2024

@isoos this is why I thought it might be necessary with some data migration, in order to just create the properties on all entities. That's something we could easily do with a backfill job, right?

Yeah, makes sense.

@spydon
Copy link
Author

spydon commented May 14, 2024

Would you be interested to implement it as such?

Sounds great! 😃

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.

Enable more automated workflows by allowing publishing from the workflow_dispatch trigger
5 participants