Skip to content
This repository has been archived by the owner on Jun 25, 2024. It is now read-only.

fix(INT-6706): check service account key in validateInvocationConfig #571

Merged
merged 2 commits into from
Feb 24, 2023

Conversation

gastonyelmini
Copy link
Contributor

Check service account key in validateInvocationConfig

Context and motivation:

In some cases, despite the fact that service account key file is setup correctly, if is not valid, the integration might run anyways and generate a large amount of errors. To avoid this, a test API call with credentials to check if they are valid and throw an error if not:

Example error that could be present:

Screen Shot 2023-01-19 at 23 11 25

Expected outcome:

Screen Shot 2023-01-19 at 23 18 05

@gastonyelmini gastonyelmini requested a review from ndowmon January 20, 2023 02:18
@gastonyelmini gastonyelmini requested a review from a team as a code owner January 20, 2023 02:18
@gastonyelmini gastonyelmini force-pushed the INT-6706-integration-key-test branch 2 times, most recently from 2128ab0 to 8ee5695 Compare January 25, 2023 13:50
@gastonyelmini gastonyelmini force-pushed the INT-6706-integration-key-test branch 5 times, most recently from 01d723b to 03dde84 Compare January 30, 2023 22:38
src/utils/parseServiceAccountKeyFile.ts Outdated Show resolved Hide resolved
src/config.test.ts Outdated Show resolved Hide resolved
@gastonyelmini gastonyelmini force-pushed the INT-6706-integration-key-test branch from 03dde84 to d641a81 Compare February 2, 2023 14:15
test/recording.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
@gastonyelmini gastonyelmini requested a review from ndowmon February 2, 2023 14:20
@gastonyelmini gastonyelmini force-pushed the INT-6706-integration-key-test branch 3 times, most recently from f4371ca to 8fc031b Compare February 10, 2023 11:41
@gastonyelmini gastonyelmini force-pushed the INT-6706-integration-key-test branch from 8fc031b to f3cf829 Compare February 10, 2023 13:32
"encoding": "utf-8",
"mimeType": "application/json; charset=UTF-8",
"size": 181,
"text": "{\"access_token\":\"[REDACTED]\",\"expires_in\":9999,\"token_type\":\"Bearer\"}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs a re-record to persist the 400 response data.

@@ -31,6 +32,7 @@ export const invocationConfig: IntegrationInvocationConfig<IntegrationConfig> =
integrationSteps: steps,
dependencyGraphOrder: ['last'],
beforeAddEntity: maybeDefaultProjectIdOnEntity,
validateInvocation,
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

ndowmon
ndowmon previously approved these changes Feb 24, 2023
Copy link
Contributor

@ndowmon ndowmon left a comment

Choose a reason for hiding this comment

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

This is awesome. Thanks!

I posted the comment about re-recording one API call. If it’s simple to re-record, please do so, but if it takes more than a few minutes, i would rather merge as-is.

🔥

@gastonyelmini gastonyelmini merged commit 25324bc into main Feb 24, 2023
@gastonyelmini gastonyelmini deleted the INT-6706-integration-key-test branch February 24, 2023 20:14
@j1-internal-automation
Copy link
Collaborator

🚀 PR was released in v2.21.0 🚀

@j1-internal-automation j1-internal-automation added the released This issue/pull request has been released. label Mar 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants