-
Notifications
You must be signed in to change notification settings - Fork 71
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
Add error mapping for Amplify app not found in specified region #2313
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 31b406a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
const appNotFoundMatch = (error as Error).message.match( | ||
/No apps found with name (?<appName>.*) in region (?<region>.*)/ | ||
); | ||
|
||
await result.writeToDirectory(out); | ||
if (appNotFoundMatch?.groups) { | ||
const { appName, region } = appNotFoundMatch.groups; | ||
throw new AmplifyUserError('AmplifyAppNotFoundError', { | ||
message: `No Amplify app found with name "${appName}" in region "${region}".`, | ||
resolution: `Ensure that an Amplify app named "${appName}" exists in the "${region}" region.`, | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My question is still not answered. How are you able to reproduce this and validate it? I don't see appName
as a user input anywhere so not sure how this can be easily categorized as a User Error.
What situation(s) can lead to this error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have moved the try-catch wrapper to a common resolver where all the generate command finally lands.I have updated the repro steps in the description. Seems to be happening only if the --branch option is set without the AppID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mostly happening in the console (based on the isCI flag in the telemetry) where customers might not be explicitly providing these values and the region should match if customers are using the Amplify Console to download the output file.
Seems to be happening only if the --branch option is set without the AppID
Yes, when only --branch
is set, the appName is loaded from the Amplify Service and filtered on the appName which is coming from the packageJson#Name (I think).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mostly happening in the console (based on the isCI flag in the telemetry) where customers might not be explicitly providing these values and the region should match if customers are using the Amplify Console to download the output file.
Input Command | CI Percentage | Non-CI Percentage |
---|---|---|
UnknownCommand | 31.03 | 68.97 |
generate graphql-client-code | 0 | 100 |
generate outputs | 56.33 | 43.67 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a valid concern, since appName is internally derived. When I tested out in the right region it works every time. When I'm working in the wrong region it fails. So we can safely call it as UserError.
This condition is triggered only when --branch is used with wrong region
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested this in the Console? I understand you can reproduce this in the CLI, but the fact that this issue happens in CI points to the issue could be something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (args.stack) { | ||
return { stackName: args.stack }; | ||
} else if (args.appId && args.branch) { | ||
return { | ||
namespace: args.appId, | ||
name: args.branch, | ||
type: 'branch', | ||
}; | ||
} else if (args.branch) { | ||
const resolvedNamespace = await this.namespaceResolver.resolve(); | ||
return { | ||
appName: resolvedNamespace, | ||
branchName: args.branch, | ||
}; | ||
} | ||
return undefined; | ||
} catch (error) { | ||
const appNotFoundMatch = (error as Error).message.match( | ||
/No apps found with name (?<appName>.*) in region (?<region>.*)/ | ||
); | ||
|
||
if (appNotFoundMatch?.groups) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which line here is throwing the exception that you are catching? Afaik this.namespaceResolver.resolve();
doesn't throw this exception, but this does
Line 42 in 3838849
`No apps found with name ${this.appNameAndBranch.appName} in region ${region}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error originates in app_name_and_branch_main_stack_name_resolver.ts
. Since we are unable to place AmplifyUserError in the deployed-backend-client, I've moved the error handling to
the CLI layer where the error bubbles up. I've successfully implemented error catching at this level. You can see in the screenshot I've added to the description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is looking good. Need to add tests as well.
const appNotFoundMatch = (error as Error).message.match( | ||
/No apps found with name (?<appName>.*) in region (?<region>.*)/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of regex matching, we should update the error thrown to have contextual information. E.g. see this example here in the deployed-backend-client
package
amplify-backend/packages/deployed-backend-client/src/backend_output_client_factory.ts
Lines 9 to 21 in 3838849
export enum BackendOutputClientErrorType { | |
METADATA_RETRIEVAL_ERROR = 'MetadataRetrievalError', | |
NO_OUTPUTS_FOUND = 'NoOutputsFound', | |
DEPLOYMENT_IN_PROGRESS = 'DeploymentInProgress', | |
NO_STACK_FOUND = 'NoStackFound', | |
CREDENTIALS_ERROR = 'CredentialsError', | |
ACCESS_DENIED = 'AccessDenied', | |
} | |
/** | |
* Error type for BackendOutputClientError | |
*/ | |
export class BackendOutputClientError extends Error { | |
public code: BackendOutputClientErrorType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
void describe('GenerateOutputsCommand error handling', () => { | ||
let clientConfigGenerator: ClientConfigGeneratorAdapter; | ||
let backendIdentifierResolver: AppBackendIdentifierResolver; | ||
let generateOutputsCommand: GenerateOutputsCommand; | ||
|
||
beforeEach(() => { | ||
// Mock the dependencies | ||
clientConfigGenerator = { | ||
generateClientConfigToFile: mock.fn(), | ||
} as unknown as ClientConfigGeneratorAdapter; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the existing describe
that is already doing all the mock setup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated it, keeping it consistent
I was planning to create Test suites separate for handling all negative test cases
await generateOutputsCommand.handler({ | ||
stack: undefined, | ||
appId: 'test-app', | ||
'app-id': 'test-app', | ||
branch: 'main', | ||
format: undefined, | ||
outDir: undefined, | ||
'out-dir': undefined, | ||
outputsVersion: '1.3', | ||
'outputs-version': '1.3', | ||
_: [], | ||
$0: 'command-name', | ||
}); | ||
assert.fail('Expected error was not thrown'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use commandRunner.runCommand
as used in other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated it
await result.writeToDirectory(out); | ||
} catch (error) { | ||
if ( | ||
error instanceof BackendOutputClientError && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per the lint, don't use instanceof as it might not work if there are multiple versions of DeployedBackendClient package in the node_modules.
import { | ||
BackendOutputClientError, | ||
BackendOutputClientErrorType, | ||
} from '../backend_output_client_factory.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BackendOutputClientError
is probably not the right error type to use here since based on it's name, it should be used with BackendOutputClient
only.
Problem
This error specifically occurs when using the following generate commands with only the --branch option:
npx ampx generate graphql-client-code --branch <branch-name>
npx ampx generate forms --branch <branch-name>
npx ampx generate outputs --branch <branch-name>
When users run these commands with only the --branch parameter and the Amplify app doesn't exist in the current AWS region, they will receive the error message:
"No Amplify app found with name '{appName}' in region '{region}'"
To properly use these commands, users should either:
For example, the correct usage would be:
Changes
Validation & Reproduction Steps
To reproduce the error scenario: