obi1kenobi / cargo-semver-checks-action

A GitHub Action for running cargo-semver-checks
MIT License
59 stars 15 forks source link

Add action outputs #64

Open orhun opened 6 months ago

orhun commented 6 months ago

Hey! 🐻

I would like to use the output of cargo-semver-checks-action in other steps/jobs as follows:

  semver:
    name: Semver
    runs-on: ubuntu-22.04
    steps:
      - name: Checkout
        uses: actions/checkout@v4

      - name: Check semver
        id: check_semver
        uses: obi1kenobi/cargo-semver-checks-action@v2
        with:
          package: git-cliff-core

      - name: Comment on PR
        if: always() && (steps.check_semver.outputs.error_message != null) && (github.event_name == 'pull_request')
        uses: marocchino/sticky-pull-request-comment@v2
        with:
          header: pr-semver-check-error
          message: |
            Thank you for opening this pull request! ⛰️

            There seems to be semver incompatibility issues reported by [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks).

            Details:

            > ${{ steps.check_semver.outputs.error_message }}

      - name: Update comment on PR
        uses: marocchino/sticky-pull-request-comment@v2
        if: (steps.check_semver.outputs.error_message == null) && (github.event_name == 'pull_request')
        with:
          header: pr-semver-check-error
          delete: true

But it does not set any outputs (i.e. outputs.error_message) so I cannot achieve this.

Inspiration: https://github.com/amannn/action-semantic-pull-request?tab=readme-ov-file#outputs

Can we get this feature in? What do you think?

obi1kenobi commented 5 months ago

Interesting idea! Would love to hear more about the workflow itself.

Is the expectation that users that open PRs containing breaking changes should immediately bump the version number in the Cargo.toml file? Would that be expected to immediately trigger a new release to be created, or can the new version number exist in the repo without being published yet?

orhun commented 5 months ago

Actually I just want to have a sticky comment in the pull request saying that there are some changes that breaks the semver compatibility. Similar to this one: https://github.com/ratatui-org/ratatui/pull/769#issuecomment-1881151750

I think this makes the error more visible and you don't have to go into the CI logs to figure out what's wrong. I especially want to have this for cargo-semver-checks because it is an important part of my CI. I run it for every pull request and try to avoid semver breakage before creating any releases.

obi1kenobi commented 5 months ago

Absolutely. I want the same thing.

In my mind, this kind of workflow would want to compare the PR branch to the target branch — e.g. pg/new-feature to main, where main is the baseline. Currently, this action only knows how to use the previous published version from crates.io as a baseline, which feels like the wrong choice — it would compare pg/new-feature to v1.2.3 from crates.io. This would would for example repeatedly flag breaking changes that have already been merged in a prior PR but haven't been released yet. This part is a solvable problem, though possibly requires a bit of annoying plumbing.

Also, in addition to the sticky comment, I'd love to have the action flag the specific lines where each issue is happening. This way you can see "this specific function wants more args" as opposed to having to look up the file and function yourself. When I last looked into this, it seemed surprisingly hard to make GitHub comments on the "left side" (the "deleted" side) of a PR. Most linters only comment on the new code so that's what GitHub supports best, but cargo-semver-checks may need to flag e.g. a function deletion in which case there isn't anything on the "additions" side of the PR to comment on. Is this something you've seen any other tool do? Is there a good approach we can use as inspiration?

Bottom line: I'd love to have this workflow work well, and I'd love to work with you on it if you'd be up for that!

orhun commented 5 months ago

this kind of workflow would want to compare the PR branch to the target branch

I agree that this should be the behavior. Currently I'm running this action for every push to the git-cliff repository and I'm getting a failure each time since it is comparing against the crates.io version. Using the main branch as the baseline would solve this problem and I'd get notified about the semver breakage only once.

This part is a solvable problem, though possibly requires a bit of annoying plumbing.

Definitely.

Also, in addition to the sticky comment, I'd love to have the action flag the specific lines where each issue is happening. [...] Is this something you've seen any other tool do? Is there a good approach we can use as inspiration?

Having this would be really nice! I think I've seen similar things but I need to think a bit - can't say a name of a tool off the top of my head now. But it should be doable I reckon.

Bottom line: I'd love to have this workflow work well, and I'd love to work with you on it if you'd be up for that!

I'm up for it! I think I can first tackle #15 - added it to my hacking list :D

What do you think?

obi1kenobi commented 5 months ago

Sounds great! Feel free to ping me anytime if a second pair of eyes could be useful.

There are two classes of problems I was worried about in #15 that would be great to have test cases for:

I still believe this is all solvable. But annoying plumbing will definitely be involved too.