From 52ae7aa0537a87cbbec071e08a2b8bda4765dbe4 Mon Sep 17 00:00:00 2001 From: Laurin Quast Date: Thu, 6 Feb 2025 17:57:17 +0100 Subject: [PATCH 1/6] fix: return github schema check error if github input is provided --- .../api/src/modules/schema/providers/schema-publisher.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/services/api/src/modules/schema/providers/schema-publisher.ts b/packages/services/api/src/modules/schema/providers/schema-publisher.ts index 2dca30ce7a..7fa908ef2d 100644 --- a/packages/services/api/src/modules/schema/providers/schema-publisher.ts +++ b/packages/services/api/src/modules/schema/providers/schema-publisher.ts @@ -346,6 +346,14 @@ export class SchemaPublisher { ) { this.logger.debug('No service name provided (type=%s)', project.type); increaseSchemaCheckCountMetric('rejected'); + + if (input.github) { + return { + __typename: 'GitHubSchemaCheckError' as const, + message: 'Missing service name provided.', + }; + } + return { __typename: 'SchemaCheckError', valid: false, From c76190db79913e045ccb18dd7ca72f9f0b7fbc86 Mon Sep 17 00:00:00 2001 From: Laurin Quast Date: Fri, 7 Feb 2025 15:36:40 +0100 Subject: [PATCH 2/6] cli fix --- .changeset/strange-shrimps-battle.md | 6 ++++++ packages/libraries/cli/src/commands/schema/check.ts | 11 +++++------ .../src/modules/schema/providers/schema-publisher.ts | 7 ------- 3 files changed, 11 insertions(+), 13 deletions(-) create mode 100644 .changeset/strange-shrimps-battle.md diff --git a/.changeset/strange-shrimps-battle.md b/.changeset/strange-shrimps-battle.md new file mode 100644 index 0000000000..8df78046ac --- /dev/null +++ b/.changeset/strange-shrimps-battle.md @@ -0,0 +1,6 @@ +--- +'@graphql-hive/cli': patch +--- + +Show correct error message when attempting a schema check on a federation project without the +`--service` paramater. diff --git a/packages/libraries/cli/src/commands/schema/check.ts b/packages/libraries/cli/src/commands/schema/check.ts index 3ce982d35a..0adca1eb1b 100644 --- a/packages/libraries/cli/src/commands/schema/check.ts +++ b/packages/libraries/cli/src/commands/schema/check.ts @@ -26,10 +26,10 @@ import { import * as TargetInput from '../../helpers/target-input'; const schemaCheckMutation = graphql(/* GraphQL */ ` - mutation schemaCheck($input: SchemaCheckInput!, $usesGitHubApp: Boolean!) { + mutation schemaCheck($input: SchemaCheckInput!) { schemaCheck(input: $input) { __typename - ... on SchemaCheckSuccess @skip(if: $usesGitHubApp) { + ... on SchemaCheckSuccess { valid initial warnings { @@ -60,7 +60,7 @@ const schemaCheckMutation = graphql(/* GraphQL */ ` webUrl } } - ... on SchemaCheckError @skip(if: $usesGitHubApp) { + ... on SchemaCheckError { valid changes { nodes { @@ -90,10 +90,10 @@ const schemaCheckMutation = graphql(/* GraphQL */ ` webUrl } } - ... on GitHubSchemaCheckSuccess @include(if: $usesGitHubApp) { + ... on GitHubSchemaCheckSuccess { message } - ... on GitHubSchemaCheckError @include(if: $usesGitHubApp) { + ... on GitHubSchemaCheckError { message } } @@ -274,7 +274,6 @@ export default class SchemaCheck extends Command { contextId: flags.contextId ?? undefined, target, }, - usesGitHubApp, }, }); diff --git a/packages/services/api/src/modules/schema/providers/schema-publisher.ts b/packages/services/api/src/modules/schema/providers/schema-publisher.ts index 7fa908ef2d..23b660ab8e 100644 --- a/packages/services/api/src/modules/schema/providers/schema-publisher.ts +++ b/packages/services/api/src/modules/schema/providers/schema-publisher.ts @@ -347,13 +347,6 @@ export class SchemaPublisher { this.logger.debug('No service name provided (type=%s)', project.type); increaseSchemaCheckCountMetric('rejected'); - if (input.github) { - return { - __typename: 'GitHubSchemaCheckError' as const, - message: 'Missing service name provided.', - }; - } - return { __typename: 'SchemaCheckError', valid: false, From adae1dc5e1ba2a0dc3a7bf95ecf9f1f18573869e Mon Sep 17 00:00:00 2001 From: Laurin Quast Date: Mon, 10 Feb 2025 09:22:05 +0100 Subject: [PATCH 3/6] add test --- integration-tests/tests/cli/schema.spec.ts | 41 ++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/integration-tests/tests/cli/schema.spec.ts b/integration-tests/tests/cli/schema.spec.ts index 64640ad977..9fe433075a 100644 --- a/integration-tests/tests/cli/schema.spec.ts +++ b/integration-tests/tests/cli/schema.spec.ts @@ -453,3 +453,44 @@ test.concurrent( `); }, ); + +test.only('schema:check gives correct error message for missing `--service` name flag in federation project', async ({ + expect, +}) => { + const { createOrg } = await initSeed().createOwner(); + const { inviteAndJoinMember, createProject } = await createOrg(); + await inviteAndJoinMember(); + const { createTargetAccessToken } = await createProject(ProjectType.Federation); + const { secret } = await createTargetAccessToken({}); + + process.env.GITHUB_ACTIONS = '1'; + process.env.GITHUB_REPOSITORY = 'foo/foo'; + onTestFinished(() => { + process.env.GITHUB_ACTIONS = undefined; + process.env.GITHUB_REPOSITORY = undefined; + }); + + await expect( + schemaCheck([ + '--registry.accessToken', + secret, + '--github', + '--author', + 'Kamil', + 'fixtures/init-schema.graphql', + ]), + ).rejects.toMatchInlineSnapshot(` + :::::::::::::::: CLI FAILURE OUTPUT ::::::::::::::: + exitCode------------------------------------------: + 1 + stderr--------------------------------------------: + › Warning: Could not resolve pull request number. Are you running this + › command on a 'pull_request' event? + › See https://__URL__ + › b-workflow-for-ci + stdout--------------------------------------------: + ✖ Detected 1 error + + - Missing service name + `); +}); From e494da103f73ae4b5553165efd400691ee5f771a Mon Sep 17 00:00:00 2001 From: Laurin Quast Date: Mon, 10 Feb 2025 09:24:30 +0100 Subject: [PATCH 4/6] oops --- integration-tests/tests/cli/schema.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration-tests/tests/cli/schema.spec.ts b/integration-tests/tests/cli/schema.spec.ts index 9fe433075a..00059c9ba5 100644 --- a/integration-tests/tests/cli/schema.spec.ts +++ b/integration-tests/tests/cli/schema.spec.ts @@ -454,7 +454,7 @@ test.concurrent( }, ); -test.only('schema:check gives correct error message for missing `--service` name flag in federation project', async ({ +test('schema:check gives correct error message for missing `--service` name flag in federation project', async ({ expect, }) => { const { createOrg } = await initSeed().createOwner(); From cdd9d5c10e5763104a771257344beb39f4e7d0d0 Mon Sep 17 00:00:00 2001 From: Laurin Quast Date: Mon, 10 Feb 2025 10:39:44 +0100 Subject: [PATCH 5/6] improve test setup --- integration-tests/testkit/cli.ts | 6 ++-- integration-tests/tests/cli/schema.spec.ts | 35 ++++++++++++---------- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/integration-tests/testkit/cli.ts b/integration-tests/testkit/cli.ts index 6328eb129f..1158078de7 100644 --- a/integration-tests/testkit/cli.ts +++ b/integration-tests/testkit/cli.ts @@ -19,12 +19,13 @@ async function generateTmpFile(content: string, extension: string) { return filepath; } -async function exec(cmd: string) { +async function exec(cmd: string, env?: Record) { const outout = await execaCommand(`${binPath} ${cmd}`, { shell: true, env: { OCLIF_CLI_CUSTOM_PATH: cliDir, NODE_OPTIONS: '--no-deprecation', + ...env, }, }); @@ -44,11 +45,12 @@ export async function schemaPublish(args: string[]) { ); } -export async function schemaCheck(args: string[]) { +export async function schemaCheck(args: string[], env?: Record) { const registryAddress = await getServiceHost('server', 8082); return await exec( ['schema:check', `--registry.endpoint`, `http://${registryAddress}/graphql`, ...args].join(' '), + env, ); } diff --git a/integration-tests/tests/cli/schema.spec.ts b/integration-tests/tests/cli/schema.spec.ts index 00059c9ba5..4550cfb9b6 100644 --- a/integration-tests/tests/cli/schema.spec.ts +++ b/integration-tests/tests/cli/schema.spec.ts @@ -454,7 +454,7 @@ test.concurrent( }, ); -test('schema:check gives correct error message for missing `--service` name flag in federation project', async ({ +test.only('schema:check gives correct error message for missing `--service` name flag in federation project', async ({ expect, }) => { const { createOrg } = await initSeed().createOwner(); @@ -463,22 +463,25 @@ test('schema:check gives correct error message for missing `--service` name flag const { createTargetAccessToken } = await createProject(ProjectType.Federation); const { secret } = await createTargetAccessToken({}); - process.env.GITHUB_ACTIONS = '1'; - process.env.GITHUB_REPOSITORY = 'foo/foo'; - onTestFinished(() => { - process.env.GITHUB_ACTIONS = undefined; - process.env.GITHUB_REPOSITORY = undefined; - }); - await expect( - schemaCheck([ - '--registry.accessToken', - secret, - '--github', - '--author', - 'Kamil', - 'fixtures/init-schema.graphql', - ]), + schemaCheck( + [ + '--registry.accessToken', + secret, + '--github', + '--author', + 'Kamil', + 'fixtures/init-schema.graphql', + ], + { + // set these environment variables to "emulate" a GitHub actions environment + // We set GITHUB_EVENT_PATH to "" because on our CI it can be present and we want + // consistent snapshot output behaviour. + GITHUB_ACTIONS: '1', + GITHUB_REPOSITORY: 'foo/foo', + GITHUB_EVENT_PATH: '', + }, + ), ).rejects.toMatchInlineSnapshot(` :::::::::::::::: CLI FAILURE OUTPUT ::::::::::::::: exitCode------------------------------------------: From bc93c229f0ab965d7ef535b0df7cf5fd419b6d12 Mon Sep 17 00:00:00 2001 From: Laurin Quast Date: Mon, 10 Feb 2025 10:43:56 +0100 Subject: [PATCH 6/6] ooops again --- integration-tests/tests/cli/schema.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration-tests/tests/cli/schema.spec.ts b/integration-tests/tests/cli/schema.spec.ts index 4550cfb9b6..84dcb9add6 100644 --- a/integration-tests/tests/cli/schema.spec.ts +++ b/integration-tests/tests/cli/schema.spec.ts @@ -454,7 +454,7 @@ test.concurrent( }, ); -test.only('schema:check gives correct error message for missing `--service` name flag in federation project', async ({ +test('schema:check gives correct error message for missing `--service` name flag in federation project', async ({ expect, }) => { const { createOrg } = await initSeed().createOwner();