semantic-release / github

:octocat: semantic-release plugin to publish a GitHub release and comment on released Pull Requests/Issues
MIT License
402 stars 125 forks source link

Invalid GQL Query in `buildAssociatedPRsQuery` when commits/shas empty #871

Closed AlexC closed 1 month ago

AlexC commented 2 months ago

During the upgrade to v10.0.7 which introduced GraphQL to the success lifecycle method, we've noticed an increase in errors in our CI pipeline and have narrowed it down to an invalid GQL Query in buildAssociatedPRsQuery as a result of the shas variable being empty (context.commits).

This may be specific to our workflow with an additional plugin, however we have the ability to force semantic release to publish a new version, which results in context.commits being empty and therefore a GQL query gets generated as below:

#graphql
query getAssociatedPRs($owner: String!, $repo: String!) {
      repository(owner: $owner, name: $repo) {
      }
 }

Resulting in:

11:56:33 GraphqlResponseError: Request failed due to following response errors:
11:56:33  - Parse error on "}" (RCURLY) at [5, 7]

It would be nice if the changes made in be394cfb9190b9cb8961e7a9155f39eb00cbde8b would handle empty context.commits as per the previous implementation.

zachspar commented 2 months ago

Bump: Having the same issue as well

gr2m commented 2 months ago

It would be nice if the changes made in be394cf would handle empty context.commits as per the previous implementation.

@babblebey can you please have a look?

zachspar commented 2 months ago

@babblebey, we need to support backward compatibility of how the old feature worked when using GraphQL instead of search API. Thanks in advance people!

travi commented 2 months ago

we have the ability to force semantic release to publish a new version

before we just move forward with "fixing" this use case, i do think we need to pause and make a decision around whether this is something that is considered supported in the first place. this is certainly not something we advertise as supported and i would argue is only accidentally supported if it ever worked.

at its core, semantic-release operates on commits. what is described here seems to break that fundamental part of what semantic-release is intended to be used for.

travi commented 2 months ago

@AlexC please provide details about the plugin you are using to accomplish this workflow. Is it a community supported, publicly available plugin, or something more custom to your team?

@zachspar i'm curious if you are using the same implementation as @AlexC. could you provide details about how you are implementing this workflow as well?

zachspar commented 2 months ago

@travi I'm using the semantic-release-monorepo extension (community supported and widely adopted solution for monorepos). IMHO regardless of whether it was an unintended feature, it still was a feature of the code and a breaking change was introduced. Usually, when a breaking change is made (regardless of unintended) a major version release should have been tagged, with limited exceptions

My workflow is based around a monorepo and publishing multiple releases to GH

I could pin a lower release version to have a temp workaround on my end, but would rather not do this haha

babblebey commented 2 months ago

My workflow is based around a monorepo and publishing multiple releases to GH

Is this repo public?? Kindly share a link to it if it is 🤔

travi commented 2 months ago

IMHO regardless of whether it was an unintended feature, it still was a feature of the code and a breaking change was introduced. Usually, when a breaking change is made (regardless of unintended) a major version release should have been tagged, with limited exceptions

we are very careful to mark breakages of supported features to our public interface as breaking. it is always possible that we make a mistake and need to revert a release and re-release as breaking, but we need to determine if there was actually a breakage before we know how we should proceed. i disagree that unintended features fall into this category, however. using a private api, for example, provides no guarantees that our changes will not result in a breakage.

we do not provide official support for monorepos, but we also want to enable community supported extensions, like semantic-release-monorepo to add support on top of our base. we need to understand more about the use case to determine if it is reasonable to handle such use cases.

I'm using the semantic-release-monorepo extension

this sounds potentially different than the ability to force publishing a new version. can you help us understand what is failing in your scenario if it is different?

zachspar commented 2 months ago

@babblebey Of course :) the link is semantic-release-monorepo.

@travi Thank you for your understanding and helping to take a look. My configuration has worked for a long time with this unintended feature, and now only recently started to break since this change :/

We are not trying to force a release, instead we're running it after a merge to main, in order invoke a normal (and valid) release. The workflow is able to successfully publish a tag and GH Release, only when it tries to getAssociatedPRs it fails, I assume this is for commenting on PRs via bot.

  [4:15:38 PM] [semantic-release] [[Function: semantic-release-monorepo]] › ℹ  No more plugins
  [4:15:38 PM] [semantic-release] › ✔  Completed step "success" of plugin "[Function: semantic-release-monorepo]"
  [4:15:38 PM] [semantic-release] › ✘  An error occurred while running semantic-release: Error: Parse error on "}" (RCURLY) at [5, 7]
...
        throw new GraphqlResponseError(
              ^

  GraphqlResponseError: Request failed due to following response errors:
   - Parse error on "}" (RCURLY) at [5, 7]
      at file:///home/runner/work/***/node_modules/@octokit/graphql/dist-bundle/index.js:89:13
      at async success (file:///home/runner/work/***/node_modules/@semantic-release/github/lib/success.js:67:28)
      at async success (file:///home/runner/work/***/node_modules/@semantic-release/github/index.js:91:3) {
    errors: [
      {
        message: 'Parse error on "}" (RCURLY) at [5, 7]',
        locations: [ { line: 5, column: 7 } ],
        pluginName: '[Function: semantic-release-monorepo]'
      }
    ],
...
      query: '#graphql\n' +
        '    query getAssociatedPRs($owner: String!, $repo: String!) {\n' +
        '      repository(owner: $owner, name: $repo) {\n' +
        '        \n' +
        '      }\n' +
        '    }\n' +
        '  ',
...
    response: {
      errors: [
        {
          message: 'Parse error on "}" (RCURLY) at [5, 7]',
          locations: [ { line: 5, column: 7 } ],
          pluginName: '[Function: semantic-release-monorepo]'
        }
      ]
    }
travi commented 2 months ago

We are not trying to force a release, instead we're running it after a merge to main, in order invoke a normal (and valid) release.

thanks for the additional detail. it is helpful to understand that you are running into the problem when processing commits as normal. i do think semantic-release-monorepo inspects the commits a bit differently than we normally do because it filters based on the file structure that each package is within the overall repository. i'm not super familiar with the implementation of semantic-release-monorepo, so it sounds like we'll need to understand how those are handled that might be different in that case. it might be helpful to open an issue within the semantic-release-monorepo repository as well, since we arent the support team for that extension and there might be better insight there.

My configuration has worked for a long time with this unintended feature

based on the usage that you describe, i'm not confident yet that "unintended" necessarily applies here. that is what we are still trying to understand. it is still possible that we have a gap in our test coverage that could be improved for a supported use case.

@babblebey Of course :) the link is semantic-release-monorepo.

this reference is appreciated. is it possible to also link to your usage? is it a public repository too?

zachspar commented 2 months ago

this reference is appreciated. is it possible to also link to your usage? is it a public repository too?

Unfortunately not public, however I can provide a snippet of the configuration yaml:

plugins:
  - '@semantic-release/commit-analyzer'
  - '@semantic-release/release-notes-generator'
  - '@semantic-release/github'

extends: semantic-release-monorepo

And we're invoking it via npx semantic-release in the same dir as .releaserc.yaml

For additional context:

v1.0.0 releases work fine, and any release after that fails when using @semantic-release/github >= 10.0.7

AlexC commented 2 months ago

@travi it is indeed a private plugin for a particular project I am involved it, however essentially it allows us to set a env var in our CI pipelines which will then force semantic-release to produce a new version even if there are no new commits.

I do agree though that this is quite a bespoke requirement from our side, and I guess we were relying on undocumented implementation detail. We have been able to work around the change in behaviour by specifying successComment: false in the config/option whenever this env var is set for us, which will effectively disable the success lifecycle here. If there's no commits, there's no way an issue/pr can be found so not possible for a success comment to be made anyway.

travi commented 2 months ago

We have been able to work around the change in behaviour by specifying successComment: false in the config/option whenever this env var is set for us, which will effectively disable the success lifecycle here.

without looking into the details of the recent implementation update, this feels important. i agree that successComment: false likely makes sense to avoid even searching for related issues/prs, so if we overlooked that with the change, i think it makes sense to resolve that. @babblebey does this sound likely, based on the change that was made?

I do agree though that this is quite a bespoke requirement from our side, and I guess we were relying on undocumented implementation detail.

if we are able to handle this situation by avoiding the call with that option, i expect that your use case will become unblocked again.

it is worth noting that such a use case is not really intended and should be considered unsupported, but we dont intend to take steps to intentionally prevent it. in general, it is worth understanding that semantic-release is not intended to be a generic release tool, but very specifically for releasing based on the semantics of the changes defined by the commits since the last release. while it may be possible to use semantic-release outside of that intention, its just worth understanding that there is a risk that some future change may break your workflow in ways that we would not be willing to undo.

babblebey commented 2 months ago

i agree that successComment: false likely makes sense to avoid even searching for related issues/prs, so if we overlooked that with the change, i think it makes sense to resolve that. @babblebey does this sound likely, based on the change that was made?

I think this is already handled, we didn't overlook for sure, In the cases where the successComment: false (as @AlexC stated they've just stated in their config), no action to search for associated PR and respective issues is performed for sure.

As seen here https://github.com/semantic-release/github/blob/master/lib%2Fsuccess.js#L57-L59

I guess the reason why this because a blocker was because the successComment is default to true hence the empty context.commit made it through.

A suggestion to address this permanently could be that we'd accompany the check with the context.commit emptiness check too 🤔

  if (successComment === false || isEmpty(context.commits) {
    //.. skipThePRs&IssueSearch
  } else {
    //..searchPRs&Issue&CommentThem
  }

I assume this should bring that backward compatibilty for @zachspar too 🤔

AlexC commented 2 months ago

@babblebey the suggestion to skip if context.commits is empty would have indeed resolved our issue

babblebey commented 2 months ago

@babblebey the suggestion to skip if context.commits is empty would have indeed resolved our issue

...and that without having to configure successComment: false, correct!?

The context.commits would be just to address it as an edge case.

AlexC commented 2 months ago

That's correct, we've worked around it by conditionally setting successComment: false so that we fall into https://github.com/semantic-release/github/blob/master/lib%2Fsuccess.js#L57-L59 - if the logic is amended to also check that context.commits is empty then this would have resolved our issue

As an aside, a lot of time was spent debugging why this was failing and where the Parse error on "}" (RCURLY) at [5, 7] was actually coming from. It seems very difficult/impossible to pass down any config options to get verbose logging out of semantic-release or Octokit. In the end I had to use patch-package to modify this plugin on deployment to add in various debugging myself

github-actions[bot] commented 1 month ago

:tada: This issue has been resolved in version 10.1.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: