Skip to content
This repository was archived by the owner on May 17, 2024. It is now read-only.

Update ruff formatter to work differently between forks and native branches #821

Merged
merged 24 commits into from
Jan 2, 2024

Conversation

sungchun12
Copy link
Contributor

@sungchun12 sungchun12 commented Dec 28, 2023

I was silly and didn't battle test my previous PR with a real fork: #815

Now this should be fixed and am proving that using this forked branch PR as an example. You'll notice the conditional github action steps in the failed jobs in previous commits: https://github.com/datafold/data-diff/actions/runs/7350229384/job/20011518084

and successful jobs: https://github.com/datafold/data-diff/actions/runs/7350235796/job/20011534959?pr=821

I verified the current state of auto committing ruff format changes works with native branches here: https://github.com/datafold/data-diff/actions/runs/7350253087/job/20011580724

Also, I updated logic to skip the motherduck integration test similar to how we skip testing some databases if the github action is kicked off from a forked branch.

Considerations

@sungchun12 sungchun12 changed the title Test-github-action-v2 Update ruff formatter to work differently between forks and native branches Dec 28, 2023
@sungchun12 sungchun12 requested a review from dlawin December 28, 2023 18:34
@sungchun12 sungchun12 self-assigned this Dec 28, 2023
@sungchun12 sungchun12 requested a review from vvkh December 29, 2023 17:56
@sungchun12 sungchun12 marked this pull request as draft December 30, 2023 02:05
@sungchun12
Copy link
Contributor Author

sungchun12 commented Dec 30, 2023

I'm going to add a labeling mechanism that maintainers are allowed to run integration tests for forked branch PRs similar to this: https://github.com/dbt-labs/dbt-snowflake/blob/main/.github/workflows/integration.yml

Edit: actually this may not solve he problem I expected. It still won't pass secrets correctly to run all the integration tests we want for forks like we do for native branches.

Edit 2: Found the elegance I was looking for with this clear example here from iterative.ai in how they test external contributions. Nice to see their battle tested examples in the wild. However, this works because they have capacity to consistently triage and examine each pull request. At our stage, I'd rather have the added friction of opening up a native branch PR (synced to the original PR's changes) to run our full test suite if the external contribution makes an update to say our snowflake connection logic. I don't want a rushed click to "approve/authorize" test runs to accidentally expose our secrets. @dlawin what's your opinion on this give you have the longest history of maintainership?

This is me walking the talk to edit 2: #812

on: pull_request_target # this is part that exposes secrets

jobs:
  authorize: # this is the human guardrail logic click "approve" 
    environment:
      ${{ github.event_name == 'pull_request_target' &&
      github.event.pull_request.head.repo.full_name != github.repository &&
      'external' || 'internal' }}
    runs-on: ubuntu-latest
    steps:
      - run: true

  test:
    needs: authorize 
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
        with:
          ref: ${{ github.event.pull_request.head.sha || github.ref }}
      - run: printenv EXAMPLE
        env:
          EXAMPLE: ${{ secrets.EXAMPLE }}

@sungchun12 sungchun12 removed the request for review from vvkh December 30, 2023 02:12
@sungchun12 sungchun12 marked this pull request as ready for review December 30, 2023 16:58
@sungchun12 sungchun12 added the bug Something isn't working label Dec 30, 2023
@sungchun12 sungchun12 merged commit e3f6315 into datafold:master Jan 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants