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

Enable manually running workflow actions #39

Merged
merged 1 commit into from
May 24, 2022
Merged

Enable manually running workflow actions #39

merged 1 commit into from
May 24, 2022

Conversation

deviant77
Copy link
Contributor

For testing builds on branches before you commit them to main.
If you merge this I can then test and send you a pull request for a working Frigate 0.11.0 beta 2 build.

For testing builds on branches other than main.
@weltenwort
Copy link
Owner

Thanks for this proposal. What keeps you from submitting a PR without this trigger? After approval from a collaborator the workflows should already be able to run.

@deviant77
Copy link
Contributor Author

Thanks for this proposal. What keeps you from submitting a PR without this trigger? After approval from a collaborator the workflows should already be able to run.

There are breaking changes in 0.11.0 so I wanted to work in a branch to test (and troubleshoot) changes to the build workflow and then use the docker image to make sure it actually worked before sending you a clean PR. It takes out the guesswork because my skills are much lower than yours. 😄
In this case I had to commit this to the main branch of my fork ("workflow_dispatch:" needs to be in the main branch) and then when I got it working, reset my main to your upstream main, which was less than ideal. 🤪

@weltenwort
Copy link
Owner

Ah, I get it - I appreciate the intent to submit clean PRs 👍 I wonder about the security implications of enabling workflow_dispatch. I tried to read up on it, but couldn't find anything. I assume it's still subject to the same contributor limitations, so it should be ok.

@weltenwort
Copy link
Owner

weltenwort commented May 23, 2022

I suspect there might be a problem with how I wrote the actions, though. The steps guarded like this would be executed in a workflow_dispatch run, even though they shouldn't:

      - name: Sign the published Docker image
        if: ${{ github.event_name != 'pull_request' }}
        env:
          COSIGN_EXPERIMENTAL: "true"
        # This step uses the identity token to provision an ephemeral certificate
        # against the sigstore community Fulcio instance.
        run: cosign sign ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}@${{ steps.build-and-push.outputs.digest }}

I guess we'll have to turn the guard into an allow-list instead of a deny-list to support it. 🤔

@weltenwort
Copy link
Owner

weltenwort commented May 23, 2022

I think it should be as simple as changing the github.event_name comparisons from != 'pull_request' to == 'push'. Would you be willing to include that in this PR?

@weltenwort weltenwort added the github_actions Pull requests that update Github_actions code label May 23, 2022
@deviant77
Copy link
Contributor Author

deviant77 commented May 23, 2022

Ah, I get it - I appreciate the intent to submit clean PRs 👍 I wonder about the security implications of enabling workflow_dispatch. I tried to read up on it, but couldn't find anything. I assume it's still subject to the same contributor limitations, so it should be ok.

Here's some GitHub documents:

The "Run workflow" button only appears in you own Actions. It won't be there for me in your repository, and it won't for you in my fork. I'm not sure there any settings to even change this limitation!

You can see a simplistic practical example here (actually where I got the idea from): https://github.com/deviant77/ipq807x-openwrt-builder
The workflow has workflow_dispatch: in it.
You have no "Run workflow" button in either my fork, or the upstream (neither do I), but if you fork it you will find the "Run workflow" button under Actions.

I suspect there might be a problem with how I wrote the actions, though. The steps guarded like this would be executed in a workflow_dispatch run, even though they shouldn't:

      - name: Sign the published Docker image
        if: ${{ github.event_name != 'pull_request' }}
        env:
          COSIGN_EXPERIMENTAL: "true"
        # This step uses the identity token to provision an ephemeral certificate
        # against the sigstore community Fulcio instance.
        run: cosign sign ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}@${{ steps.build-and-push.outputs.digest }}

I guess we'll have to turn the guard into an allow-list instead of a deny-list to support it. 🤔

I think your actions are correct.
With your current workflow actions and the workflow_dispatch: PR it makes the following possible:

  1. In my fork I create a branch to work on.
  2. I make some commits in my branch.
  3. When ready, I run the workflow manually on my branch.
  4. It either fails and I troubleshoot and make more commits, or it's successful and I have a docker image in my repository which I can test, then if testing is okay...
  5. I create a nice clean tested PR from my branch to your repository.
  6. Your workflow checks my PR but doesn't push or sign (as it does now).
  7. If it's successful (which it should be) you merge the PR to main and the workflow builds and publishes (as it does now)!

I think it should be as simple as changing the github.event_name comparisons from != 'pull_request' to == 'push'. Would you be willing to include that in this PR?

I think != 'pull_request' is okay because you've specified the workflow only triggers on push and pull requests on the "main" branch where you're merging the PRs to.

So in summary I think everything is fine as it is, we just need to enable the "Run workflow" button by merging this PR to main! 😄

Copy link
Owner

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for patiently walking me through this. My mental model of what the docker image tag will be when the trigger is workflow_dispatch is incomplete, which is why I was hesitant. But let's give it a try and iterate on it if doesn't behave quite right.

Thank you! ❤️

@weltenwort weltenwort merged commit f24456b into weltenwort:main May 24, 2022
@deviant77 deviant77 deleted the enable-manual-run-workflow-actions branch June 10, 2022 16:41
@deviant77
Copy link
Contributor Author

I've only just got builds working on my fork!
They were working when I added workflow_dispatch to main in my fork, but after you merged this PR and I re-created my fork, the builds were mysteriously failing with a permissions error. I couldn't work it out, and decided to come back to it "later". 🤪
I just deleted the package (even though it was showing as linked to my fork and had correct permissions), and ran a build manually and it re-created the package and all successful now.

I've just created a branch named "this-is-a-test-branch" and ran a manual build.
You can see the resulting package here.
The docker image tag is still the name of the branch (in this case frigate-synology-dsm7:this-is-a-test-branch), which I think is good.

@weltenwort
Copy link
Owner

good to hear, thanks for sharing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update Github_actions code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants