Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support workflow_dispatch event in GitHub Action #311

Merged
merged 7 commits into from
Aug 2, 2021

Conversation

lyleunderwood
Copy link
Contributor

@lyleunderwood lyleunderwood commented Mar 18, 2021

#307

I was trying to setup a manual chromatic runner using github actions via a github bot, and I wanted to use workflow_dispatch but this action didn't seem to allow it. I went ahead and added basic support.

@lyleunderwood lyleunderwood force-pushed the workflow-dispatch branch 2 times, most recently from 5a41fc2 to bb386c7 Compare March 20, 2021 00:08
@lyleunderwood
Copy link
Contributor Author

It seems like chromatic only ever wants just the branch name, never the full ref (refs/heads/my-branch). Providing the full ref seems to result in chromatic always failing to compare to master. Does anybody have any insight into this point? Is there any reason I'd ever want to allow the full ref to pass through?

@ghengeveld ghengeveld requested a review from ndelangen April 24, 2021 09:10
@ndelangen
Copy link
Member

@lyleunderwood could you elaborate this question:

Is there any reason I'd ever want to allow the full ref to pass through?

@ghengeveld
Copy link
Member

ghengeveld commented May 21, 2021

@lyleunderwood That's right, we basically never expect to see refs/heads/. We strip that off right here. Arguably the way we do it is slightly safer because the regex ensures it only strips it at the start of the string, never somewhere half way. It's unlikely someone would have an actual branch name like that, but theoretically possible.

@ndelangen Can you judge whether workflow_dispatch is safe for us to allow? If so, I think this is a nice improvement.

@ndelangen
Copy link
Member

Can you judge whether workflow_dispatch is safe for us to allow? If so, I think this is a nice improvement.

Shall we add a test workflow in this repo and test it.

@lyleunderwood
Copy link
Contributor Author

Hey, sorry I've been on vacation 🍹

I went ahead and updated the README to reflect that we really just want a regular branch name.

As for stripping off the ref prefix, it's currently just based on what's being done in the push case, let me know if I should remove it.

branch: event.payload.ref.replace('refs/heads/', ''),

@Manubi
Copy link

Manubi commented Jun 22, 2021

Will this get merged any time soon? :)

@ghengeveld ghengeveld changed the title Add support to action for workflow_dispatch Support workflow_dispatch trigger in GitHub Action Aug 2, 2021
@ghengeveld ghengeveld changed the title Support workflow_dispatch trigger in GitHub Action Support workflow_dispatch event in GitHub Action Aug 2, 2021
@ghengeveld ghengeveld merged commit 4e229b9 into chromaui:next Aug 2, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants