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

fix: replace searchAPI usage with GraphQL in findSRIssue util #907

Merged
merged 38 commits into from
Sep 20, 2024

Conversation

babblebey
Copy link
Member

@babblebey babblebey commented Aug 28, 2024

This Pull request replaces the usage of the troublesome 😉 GitHub SearchAPI as primary logic in the findSRIssue with graphql. It relegates the searchAPI consumption to a fallback logic that will be removed in a future release.

The GraphQL functionality performs its search for Semantic Release Issue by querying for Issues on a repository with specific filter, in our case we use a label in the filter. This means that the PR introduces changes to the way we "create" and "find" Sematic Release issues in the fail and success script.

Changes Made:

  • Introduced an opinionated label for Semantic Release failing release issues in form of a constant RELEASE_FAIL_LABEL

  • Concatenated the RELEASE_FAIL_LABEL to the label param on creation of semantic release failing issue in the fail script; this means the our opinionated label gets added to the failing release issue regardless of the plugin context.label configuration value (which is either semantic-release, CUSTOM_LABEL or false)

    const newIssue = {
      owner,
      repo,
      title: failTitle,
      body: `${body}\n\n${ISSUE_ID}`,
      labels: (labels || []).concat([RELEASE_FAIL_LABEL]), // 👈 👈 👈 👈  HERE
      assignees,
    };
  • Introduced the GraphQL Query that the appropriated filter and passed into it is the labels filter with context.label configuration concatenated with our opinionated RELEASE_FAIL_LABEL as value;

    const {
      repository: {
        issues: { nodes: issueNodes },
      },
    } = await octokit.graphql(loadGetSRIssuesQuery, {
      owner,
      repo,
      filter: {
        labels: (labels || []).concat([RELEASE_FAIL_LABEL]), // 👈 👈 👈 👈  HERE
      },
    });
  • Introduced error handling to the relegated GitHub searchAPI consumption; this allows for non-stop flow of step when an error occurs in the request to the hectic 😆 endpoint

  • Converted (as initially said) GitHub searchAPI consumption to fallback logic which triggers when no fail issue is found through newly established means of finding fail issues

  • Introduced the opinionated RELEASE_FAIL_LABEL to the failing release issues at creation as imperative label (no customizability)

Related Issue

Fixes #867

Screencast

DEMO: Closing all Opened Semantic Release Issues (with semantic-release label, RELEASE_FAIL_LABEL opinionated label and without any label)

screencast-bpconcjcammlapcogcnnelfmaeghhagj-2024.7.mp4

Copy link
Member Author

@babblebey babblebey left a comment

Choose a reason for hiding this comment

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

BLOCKED HERE! HELP 🫨 📣

Everything works just as expected giving the PR a whirl.... But in the integration test coverage; the case below is bloody frustrating me 😭

test("Verify, release and notify success", async (t) => {
const owner = "test_user";
const repo = "test_repo";
const env = { GITHUB_TOKEN: "github_token" };
const assets = [
"upload.txt",
{ path: "upload_other.txt", name: "other_file.txt", label: "Other File" },
];
const failTitle = "The automated release is failing 🚨";
const options = {
publish: [
{ path: "@semantic-release/npm" },
{ path: "@semantic-release/github", assets },
],
repositoryUrl: `https://github.com/${owner}/${repo}.git`,
};
const nextRelease = {
gitTag: "v1.0.0",
name: "v1.0.0",
notes: "Test release note body",
};
const releaseUrl = `https://github.com/${owner}/${repo}/releases/${nextRelease.version}`;
const assetUrl = `https://github.com/${owner}/${repo}/releases/download/${nextRelease.version}/upload.txt`;
const otherAssetUrl = `https://github.com/${owner}/${repo}/releases/download/${nextRelease.version}/other_file.txt`;
const releaseId = 1;
const uploadOrigin = "https://github.com";
const uploadUri = `/api/uploads/repos/${owner}/${repo}/releases/${releaseId}/assets`;
const uploadUrl = `${uploadOrigin}${uploadUri}{?name,label}`;
const prs = [{ number: 1, pull_request: {}, state: "closed" }];
const commits = [{ hash: "123", message: "Commit 1 message" }];
const fetch = fetchMock
.sandbox()
.get(
`https://api.github.local/repos/${owner}/${repo}`,
{
permissions: { push: true },
full_name: `${owner}/${repo}`,
clone_url: `htttps://api.github.local/${owner}/${repo}.git`,
},
{
repeat: 2,
},
)
.postOnce("https://api.github.local/graphql", {
data: {
repository: {
commit123: {
oid: "123",
associatedPullRequests: {
pageInfo: {
endCursor: "NI",
hasNextPage: false,
},
nodes: [prs[0]],
},
},
},
},
})
.postOnce(
`https://api.github.local/repos/${owner}/${repo}/releases`,
{
upload_url: uploadUrl,
html_url: releaseUrl,
id: releaseId,
},
{
body: {
tag_name: nextRelease.gitTag,
name: nextRelease.name,
body: nextRelease.notes,
draft: true,
prerelease: false,
},
},
)
.patchOnce(
`https://api.github.local/repos/${owner}/${repo}/releases/${releaseId}`,
{ html_url: releaseUrl },
{ body: { draft: false } },
)
.getOnce(
`https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`,
[{ sha: commits[0].hash }],
)
.postOnce(
`https://api.github.local/repos/${owner}/${repo}/issues/1/labels`,
{},
{ body: ["released"] },
)
.postOnce(
(_, { body }) => {
t.regex(JSON.parse(body).query, /query getSRIssues\(/);
return true;
},
{
data: {
repository: {
issues: { nodes: [] },
},
},
},
)
.getOnce(
`https://api.github.local/search/issues?q=${encodeURIComponent(
"in:title",
)}+${encodeURIComponent(`repo:${owner}/${repo}`)}+${encodeURIComponent(
"type:issue",
)}+${encodeURIComponent("state:open")}+${encodeURIComponent(failTitle)}`,
{ items: [] },
)
.postOnce(
`${uploadOrigin}${uploadUri}?name=${encodeURIComponent("upload.txt")}&`,
{ browser_download_url: assetUrl },
)
.postOnce(
`${uploadOrigin}${uploadUri}?name=other_file.txt&label=${encodeURIComponent(
"Other File",
)}`,
{
browser_download_url: otherAssetUrl,
},
)
.postOnce(
`https://api.github.local/repos/${owner}/${repo}/issues/1/comments`,
{
html_url: "https://github.com/successcomment-1",
},
);
await t.notThrowsAsync(
t.context.m.verifyConditions(
{},
{ cwd, env, options, logger: t.context.logger },
{
Octokit: TestOctokit.defaults((options) => ({
...options,
request: { ...options.request, fetch },
})),
},
),
);
await t.context.m.publish(
{ assets },
{
cwd,
env,
options,
branch: { type: "release", main: true },
nextRelease,
logger: t.context.logger,
},
{
Octokit: TestOctokit.defaults((options) => ({
...options,
request: { ...options.request, fetch },
})),
},
);
await t.context.m.success(
{ assets, failTitle },
{
cwd,
env,
options,
nextRelease,
commits,
releases: [],
logger: t.context.logger,
},
{
Octokit: TestOctokit.defaults((options) => ({
...options,
request: { ...options.request, fetch },
})),
},
);
t.deepEqual(t.context.log.args[0], ["Verify GitHub authentication"]);
t.true(t.context.log.calledWith("Published file %s", otherAssetUrl));
t.true(t.context.log.calledWith("Published file %s", assetUrl));
t.true(t.context.log.calledWith("Published GitHub release: %s", releaseUrl));
t.true(fetch.done());
});

Here's what I've got:

In the context of this particular test case, I have only made 2 changes, and they're stated below...

  • Introduced a graphql request mock to intercept the graphql request in findSRIssues; did this with a function matcher option ensuring no conflict with the existing graphql request mock for fetching associatedPRs

    .postOnce(
      // Matcher Starts
      (_, { body }) => {
        t.regex(JSON.parse(body).query, /query getSRIssues\(/);
        return true;
      },
      // Matcher Ends
      {
        data: {
          repository: {
            issues: { nodes: [] },
          },
        },
      },
    )
  • THE SECOND CHANGE DYNAMICS: This change introduced different matcher to the associatedPRs graphql request mock and it leads to different errors based on the different matcher, state below...

    .postOnce(/MATCHER_HERE/, {
      data: {
        repository: {
          commit123: {
            oid: "123",
            associatedPullRequests: {
              pageInfo: {
                endCursor: "NI",
                hasNextPage: false,
              },
              nodes: [prs[0]],
            },
          },
        },
      },
    })
    • When matcher is plain old "https://api.github.local/graphql" (currently commited change); See current full error at https://github.com/semantic-release/github/actions/runs/10619388030/job/29436858061?pr=907#step:8:94 or see snippet below

      ```
      [test:integration]   HttpError (RequestError) {
      [test:integration]     cause: SyntaxError {
      [test:integration]       message: '"undefined" is not valid JSON',
      [test:integration]     },
      [test:integration]     request: {
      [test:integration]       body: {
      [test:integration]         body: 'Test release note body',
      [test:integration]         draft: true,
      [test:integration]         name: 'v1.0.0',
      [test:integration]         prerelease: false,
      [test:integration]         tag_name: 'v1.0.0',
      [test:integration]       },
      [test:integration]       headers: {
      [test:integration]         accept: 'application/vnd.github.v3+json',
      [test:integration]         authorization: 'token [REDACTED]',
      [test:integration]         'content-type': 'application/json; charset=utf-8',
      [test:integration]         'user-agent': 'test octokit-core.js/6.1.2 Node.js/20.9.0 (win32; x64)',
      [test:integration]       },
      [test:integration]       method: 'POST',
      [test:integration]       request: {
      [test:integration]         agent: undefined,
      [test:integration]         fetch: Function fetchMockProxy { … },
      [test:integration]         hook: Function bound bound register {},
      [test:integration]       },
      [test:integration]       url: 'https://api.github.local/repos/test_user/test_repo/releases',
      [test:integration]     },
      [test:integration]    
      ```
      
    • When matcher uses a function as seen below (this set-up similar to the matcher used in the mock request for findSRIssue uses digs into the request object an matches to intercepts request where query getAssociatedPRs string is present in the query property)

      (_, { body }) => {
        t.regex(JSON.parse(body).query, /query getAssociatedPRs\(/);
        return true;
      },

      Error:

      [test:integration]    685:     .postOnce((_, { body }) => {
      [test:integration]    686:       t.regex(JSON.parse(body).query, /query getAssociatedPRs\(/);
      [test:integration]    687:       return true;
      [test:integration]
      [test:integration]   `t.regex()` must be called with a string
      [test:integration]
      [test:integration]   Called with:
      [test:integration]
      [test:integration]   undefined
      
    • When matcher uses same function and asserts the request url, seen below

      (url, { body }) => {
        t.is(url, "https://api.github.local/graphql");
        t.regex(JSON.parse(body).query, /query getAssociatedPRs\(/);
        return true;
      }

      Error:

      [test:integration]    685:     .postOnce((url, { body }) => {
      [test:integration]    686:       t.is(url, "https://api.github.local/graphql");
      [test:integration]    687:       t.regex(JSON.parse(body).query, /query getAssociatedPRs\(/);
      [test:integration]
      [test:integration]   Difference (- actual, + expected):
      [test:integration]
      [test:integration]   - 'https://api.github.local/repos/test_user/test_repo/releases'
      [test:integration]   + 'https://api.github.local/graphql'
      
    • When matcher uses another supported function format, seen below

      (_, __, { body }) => {
        t.regex(JSON.parse(body).query, /query getAssociatedPRs\(/);
        return true;
      }

      Error:

      [test:integration]   HttpError (RequestError) {
      [test:integration]     cause: TypeError {
      [test:integration]       message: 'Cannot destructure property \'body\' of \'undefined\' as it is undefined.',
      [test:integration]     },
      [test:integration]     request: {
      [test:integration]       body: {
      [test:integration]         body: 'Test release note body',
      [test:integration]         draft: true,
      [test:integration]         name: 'v1.0.0',
      [test:integration]         prerelease: false,
      [test:integration]         tag_name: 'v1.0.0',
      [test:integration]       },
      [test:integration]       headers: {
      [test:integration]         accept: 'application/vnd.github.v3+json',
      [test:integration]         authorization: 'token [REDACTED]',
      [test:integration]         'content-type': 'application/json; charset=utf-8',
      [test:integration]         'user-agent': 'test octokit-core.js/6.1.2 Node.js/20.9.0 (win32; x64)',
      [test:integration]       },
      [test:integration]       method: 'POST',
      [test:integration]       request: {
      [test:integration]         agent: undefined,
      [test:integration]         fetch: Function fetchMockProxy { … },
      [test:integration]         hook: Function bound bound register {},
      [test:integration]       },
      [test:integration]       url: 'https://api.github.local/repos/test_user/test_repo/releases',
      [test:integration]     },
      [test:integration]     response: undefined,
      [test:integration]     status: 500,
      [test:integration]     message: 'Cannot destructure property \'body\' of \'undefined\' as it is undefined.',
      [test:integration]   }
      

Important Note

Delete the test case "Verify, release and notify success" itself 🫣 and every other 242 tests are passes.... I'm very convinced something is messed up in there that I need to figure out 😢

Cc @gr2m @travi

@babblebey
Copy link
Member Author

BLOCKED HERE! HELP 🫨 📣

Everything works just as expected giving the PR a whirl.... But in the integration test coverage; the case below is bloody frustrating me 😭

Yoooooo, I addressed it 🥳🥳🥳🥳

What I did 🤌

Its important to note that in my function matchers, I implemented assertion as seen below....

(url, { body }) => {
  t.is(url, "https://api.github.local/graphql");
  t.regex(JSON.parse(body).query, /query getAssociatedPRs\(/);
  return true;
}

...well this works in some test cases and doesn't work in this particular troublesome case... But on a quick call with @gr2m today, I learnt I could/should just do away with assertions in the matchers... Hence I implement plain condition that return true when it finds a match for the mock... see below

(url, { body }) =>
  url === "https://api.github.local/graphql" &&
  JSON.parse(body).query.includes("query getAssociatedPRs("),

...and voila, the bad boy works 😁.

I was also experiencing similar troubles in #874 and all in now well 😉... I guess I can have a good weekend now 😃

@babblebey babblebey marked this pull request as ready for review August 30, 2024 20:33
@babblebey babblebey requested a review from a team August 30, 2024 20:41
lib/resolve-config.js Outdated Show resolved Hide resolved
lib/resolve-config.js Outdated Show resolved Hide resolved
Copy link
Member

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Let's gooo! We discussed this extensively and @babblebey tested it locally, time to ship 🚀

@babblebey babblebey merged commit 7fb46a3 into master Sep 20, 2024
6 checks passed
@babblebey babblebey deleted the feat/gql-for-search-api branch September 20, 2024 18:16
Copy link

🎉 This PR is included in version 10.3.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@zachspar
Copy link

zachspar commented Sep 24, 2024

@babblebey the issue no longer fails my pipeline 👍, but why is it still using GH SearchAPI at all as a fallback (it eats up time in pipeline)? Shouldn't the new mechanism of finding issues be sufficient? In my case, no issue was found using GraphQL, so it still went to old logic as fallback?

Thanks in advance!

  Request quota exhausted for request GET /search/issues
  Request quota exhausted for request GET /search/issues
  Request quota exhausted for request GET /search/issues
  Request quota exhausted for request GET /search/issues
  Request quota exhausted for request GET /search/issues
  2024-09-24T16:49:06.750Z semantic-release:github found semantic-release issues: []
  [4:49:06 PM] [semantic-release] › ℹ  An error occured fetching issue via fallback (with GH SearchAPI)

@babblebey
Copy link
Member Author

Hey @zachspar,

Great question. The SearchAPI was used to search "semantic-release fail issue" using just the title, its a little different from how we are now searching with GraphQL which searches list of issues on a repo using labels... In order to enable empower this label search it meant that we had to change how we create the "semantic-release fail issue" in the first place, we did this by enforcing addition of an opinionated RELEASE_FAIL_LABEL which we will always filter with at search.

The need for SearchAPI has fallback arises in cases when their are "semantic-release fail issue" created by the old setup (these are issues that didn't get the opinionated RELEASE_FAIL_LABEL added at creation), so this fallback only triggers to ensure that these old issues are still caught if they exist.

@zachspar
Copy link

The need for SearchAPI has fallback arises in cases when their are "semantic-release fail issue" created by the old setup (these are issues that didn't get the opinionated RELEASE_FAIL_LABEL added at creation), so this fallback only triggers to ensure that these old issues are still caught if they exist.

Gotcha, so it is for backward compatibility. Will there be any plans to deprecate the fallback logic? Or is there a mechanism it could use to search for these issues using GraphQL without labels?

@babblebey
Copy link
Member Author

Will there be any plans to deprecate the fallback logic?

Says in there 😉

/**
* BACKWARD COMPATIBILITY: Fallback to the search API if the issue was not found in the GraphQL response.
* This fallback will be removed in a future release
*/

Or is there a mechanism it could use to search for these issues using GraphQL without labels?

None at the moment, but if we do find any GraphQL means that works as simply as we used the SearchAPI with title... that would be great and would mean we can completely deprecate usage of the SearchAPI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exceeding a secondary rate limit even with 10.0.7
3 participants