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 dependabot PRs #556

Merged
merged 11 commits into from
Oct 5, 2022
Merged

🧹 Enable dependabot PRs #556

merged 11 commits into from
Oct 5, 2022

Conversation

czunker
Copy link
Contributor

@czunker czunker commented Sep 13, 2022

Signed-off-by: Christian Zunker christian@mondoo.com

with:
service-account-credentials: ${{ secrets.MONDOO_CLIENT }}
path: "mondoo-operator-manifests.yaml"
secrets: inherit
integration-tests:
needs: [unit-tests]
uses: ./.github/workflows/integration-tests.yaml
Copy link
Member

Choose a reason for hiding this comment

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

Do we know for sure this works? I think with this setup we will have the same issue with the secrets not being present

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure.

According to the docs, the workflow_call jobs have access to secrets. But I'm not sure what in this combination the secrets: inherit does. I couldn't find anything about this combination. In case this does not work, we can call the integration-tests and security-tests differently and try that.

For testing the secrets, I wanted to recreate this dependabot PR: #554

Copy link
Member

Choose a reason for hiding this comment

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

I am almost certain that this won't change anything. If you use secrets: inherit, it means the child workflow is getting the secrets from the parent workflow. If the tests are running from a fork or for a dependabot branch, they have no access to the secrets. The integration tests won't have access either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched to pull_request_target + checkout head. Now the jobs have access to the secrets.

But this also means we trust dependabot because the Jobs are now running with the code of the PR.

@czunker czunker changed the title 🧹 Enable fork PRs 🧹 Enable dependabot PRs Sep 14, 2022
@czunker
Copy link
Contributor Author

czunker commented Sep 14, 2022

I changed this PR to only allow dependabot. We'll give the Settings described in #553 a try.

@czunker czunker force-pushed the christian/allow_fork_prs branch from 20d0597 to 3d96126 Compare September 20, 2022 13:21
@czunker
Copy link
Contributor Author

czunker commented Sep 20, 2022

So, next take on this problem.

According to the docs:

The workflow started by the workflow_run event is able to access secrets and write tokens, even if the previous workflow was not.

@@ -11,9 +11,13 @@ on:
jobs:
unit-tests:
runs-on: ubuntu-latest
if: github.actor == 'dependabot[bot]' || !github.event.pull_request_target.head.repo.fork
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we remove this line, even fork PRs should work. We could handle their approval with GitHub security settings as mentioned in #553 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

We would block the workflows from running via the approvals/tags on the PR then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.
@benr has activated the security settings mentioned in the comment. I understand we would see an approval button for dependabot PRs and fork PRs when this line is gone.

czunker added 11 commits October 4, 2022 10:23
Fixes #553

Signed-off-by: Christian Zunker <christian@mondoo.com>
Signed-off-by: Christian Zunker <christian@mondoo.com>
Signed-off-by: Christian Zunker <christian@mondoo.com>
Signed-off-by: Christian Zunker <christian@mondoo.com>
Signed-off-by: Christian Zunker <christian@mondoo.com>
Signed-off-by: Christian Zunker <christian@mondoo.com>
Signed-off-by: Christian Zunker <christian@mondoo.com>
Signed-off-by: Christian Zunker <christian@mondoo.com>
Signed-off-by: Christian Zunker <christian@mondoo.com>
Because of:
Error: reusable workflows are currently not supported (see nektos/act#826 for updates)

Act does not support our workflow structure

Signed-off-by: Christian Zunker <christian@mondoo.com>
Signed-off-by: Christian Zunker <christian@mondoo.com>
@czunker czunker force-pushed the christian/allow_fork_prs branch from 3fa8b47 to 009e747 Compare October 4, 2022 08:26
Copy link
Contributor

@joelddiaz joelddiaz left a comment

Choose a reason for hiding this comment

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

can't wait for this one...let's see how this goes

@czunker czunker merged commit 8b9a08b into main Oct 5, 2022
@czunker czunker deleted the christian/allow_fork_prs branch October 5, 2022 15:57
@czunker czunker mentioned this pull request Oct 5, 2022
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.

3 participants