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

Breaking check fails on new proto files #81

Closed
iCynara opened this issue Nov 29, 2024 · 5 comments
Closed

Breaking check fails on new proto files #81

iCynara opened this issue Nov 29, 2024 · 5 comments

Comments

@iCynara
Copy link

iCynara commented Nov 29, 2024

We have a GH action scan changed proto file and run bufbuild/buf-action on it.
But if the PR only contain a new added proto file, breaking fails with error

Failure: no .proto files were targeted. This can occur if no .proto files are found in your input, --path points to files that do not exist, or --exclude-path excludes all files.

I think this is because breaking try to pull the same file on the older version which does exist.

Here is our original GH action

name: Protobuf Lint
on:
  pull_request:
    types: [opened, synchronize, reopened, labeled, unlabeled]
permissions:
  contents: read
  pull-requests: write
jobs:
  buf:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683
        with:
          fetch-depth: 0
      - name: Get changed .proto files
        id: changed-protos
        run: |
          git fetch origin ${{ github.base_ref }}
          echo 'changed_files<<EOF' >> $GITHUB_OUTPUT
          git diff --name-only HEAD $(git merge-base HEAD origin/main) -- '*.proto' >> $GITHUB_OUTPUT
          echo 'EOF' >> $GITHUB_OUTPUT
      - uses: bufbuild/buf-action@3fb70352251376e958c4c2c92c3818de82a71c2b
        if: steps.changed-protos.outputs.changed_files != ''
        with:
          push: false
          paths: ${{ steps.changed-protos.outputs.changed_files }}

To fix that i did a work around (because i don't want people to use buf skip breaking if they have new files and modified files)

Separated changed files and new files in GH action. And add `breaking: false for new files.

  - name: Get new .proto files
        id: added-protos
        run: |
          git fetch origin ${{ github.base_ref }}
          echo 'added_files<<EOF' >> $GITHUB_OUTPUT
          git diff --name-only --diff-filter=A $(git merge-base HEAD origin/main)...HEAD -- '*.proto' >> $GITHUB_OUTPUT
          echo 'EOF' >> $GITHUB_OUTPUT
          echo $GITHUB_OUTPUT
      - uses: bufbuild/buf-action@3fb70352251376e958c4c2c92c3818de82a71c2b
        if: steps.added-protos.outputs.added_files != ''
        with:
          push: false
          breaking: false
          paths: ${{ steps.added-protos.outputs.added_files }}

My question is if this is what we suppose to do? Adding new file is very common and i feel like it should be natively supported and we just didn't use it right?

@nicksnyder
Copy link
Member

You shouldn't need to set the paths: field at all, unless you have some special needs. Can you see if removing the path: field solves your problem?

@emcfarlane
Copy link
Collaborator

@iCynara I agree with Nick and think the paths restriction here is what is causing issues. There shouldn't be a need to run checks on only the changeset. Using the paths will restrict the module build input and I believe cause the breaking changes as files not modified will appear to be deleted. Adding new files is not a breaking change. Could you provide a bit more details on why you needed to set paths?

@iCynara
Copy link
Author

iCynara commented Dec 5, 2024

Hey I think you guys are right if i don't do path it will not break on breaking check .
The reason we added it because we already have whole bunch of protos in the repo before we add linter.
And i think we only want to check new protos instead of backfill all the existing ones, that's why we only check the changed files there

Also we may want to check certain folder only , example we have

 - experimental_proto  
  \_ foo.proto
 - public
  \_ a_real_proto.proto

We would want to do that with git diff
git diff --name-only $(git merge-base HEAD origin/main)...HEAD -- 'public/*.proto' >> $GITHUB_OUTPUT

or probably should do

paths:
 - 'public/*.proto'

then we will still be using paths if that's the one causing trouble.

@emcfarlane
Copy link
Collaborator

@iCynara I think what you'll want is to update the buf.yaml to ignore directories with the ignore parameter or certain checks with ignore_only. This has the advantage that developers can simply run buf lint locally without specifying the list of paths to their changes. Let me know if that works for you!

@iCynara
Copy link
Author

iCynara commented Dec 9, 2024

We are okay with using my workaround for now. But thank you for all of you provide many different ways to do it. 🙇🏼‍♀️

@iCynara iCynara closed this as completed Dec 9, 2024
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

No branches or pull requests

3 participants