correctly handle stack argument for generate schema command #2028
Conversation
🦋 Changeset detectedLatest commit: 46dc9b3 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 backendIdentifierIsStack = 'stackName' in backendIdentifier; | ||
| const sanitizedBackendId = backendIdentifierIsStack | ||
| ? BackendIdentifierConversions.fromStackName(backendIdentifier.stackName) | ||
| : backendIdentifier; | ||
|
|
There was a problem hiding this comment.
Can this be turned into a convertDeployedBackendIdentifierToBackendIdentifier? Because we have two types DeployedBackendIdentifier and BackendIdentifier this confusion or issue may happen else where as well.
There was a problem hiding this comment.
Or perhaps we should refactor this and have.
BackendIdentifierResolver.resolveDeployedBackendIdentifier
and
BackendIdentifierResolver.resolveBackendIdentifier
so that this conversion logic stays in one place.
these are all internal types to cli package so we could do that.
There was a problem hiding this comment.
I'm leaning towards keeping it all in BackendIdentifierResolver for now, we can determine later if there is a need to separate this logic.
| const connectionUriSecret = await this.secretClient.getSecret( | ||
| backendIdentifier as BackendIdentifier, | ||
| sanitizedBackendId as BackendIdentifier, |
There was a problem hiding this comment.
We should remove this force cast once we have an instance of BackendIdentifier
There was a problem hiding this comment.
Looks like type system was trying to warn about the problem in original implementation.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
packages/cli/src/backend-identifier/backend_identifier_with_sandbox_fallback.ts
Fixed
Show fixed
Hide fixed
| resolveDeployedBackendIdToBackendId: ( | ||
| deployedBackendId?: DeployedBackendIdentifier | ||
| ) => Promise<BackendIdentifier | undefined>; |
There was a problem hiding this comment.
| resolveDeployedBackendIdToBackendId: ( | |
| deployedBackendId?: DeployedBackendIdentifier | |
| ) => Promise<BackendIdentifier | undefined>; | |
| resolveDeployedBackendIdToBackendId: ( | |
| deployedBackendId: DeployedBackendIdentifier | |
| ) => Promise<BackendIdentifier>; |
Should we have caller handle undefined ?
Or perhaps should we have
resolveBackendIdentifier: (
args: BackendIdentifierParameters
) => Promise<BackendIdentifier | undefined>;
as additional API ?
The thing is that if callers give this type BackendIdentifierParameters they should be getting final product instead of passing return value from one API to the other one - that solution is not much beneficial over just calling "Conversions".
I like the later more as I type this.
There was a problem hiding this comment.
I'm all for the latter, it just makes me question the use of the original resolve for other commands but looks like those are different and do require DeployedBackendIdentifier.
There was a problem hiding this comment.
for other commands but looks like those are different and do require DeployedBackendIdentifier.
Yep. that's why that api has to stay. Consider renaming it though to make it less confusing.
| args | ||
| ); | ||
| const backendIdentifier = | ||
| await this.backendIdentifierResolver.resolveBackendIdentifier(args); | ||
|
|
||
| if (!backendIdentifier) { | ||
| throw new AmplifyFault('BackendIdentifierFault', { |
There was a problem hiding this comment.
Should this be a fault if the input is coming from the user?
There was a problem hiding this comment.
I think this should stay as a fault because if we can't resolve using input from the user we fallback to resolving to sandbox backend ID. The only thing really controlled by the user at that point is if they have the name field in their package.json which is covered here https://github.com/aws-amplify/amplify-backend/blob/main/packages/cli/src/backend-identifier/local_namespace_resolver.ts#L27.
There was a problem hiding this comment.
This is concerning. if a user provided a --stack argument that doesn't result in a valid identifier, why would we fallback to sandbox backend identifier? This seems like a wrong fallback when customer intention is clear.
There was a problem hiding this comment.
True... this fallback is there for all generate commands unfortunately
There was a problem hiding this comment.
I can follow up with a PR to not fallback if user provided stack or app ID and branch then change this to an error
There was a problem hiding this comment.
Sounds good. I think in some context it might makes sense to have this fallback but maybe not in the cli commands.
Problem
When passing stack option for
generate schema-from-databasecommand, secret clientgetSecretis expecting backend identifier to be like https://github.com/aws-amplify/amplify-backend/blob/main/packages/plugin-types/src/backend_identifier.ts#L17 but it is resolved to{ stackName: '<stack>' }. So we fail to convert the backend identifier to parameter path strings for connection URI and SSL Cert secrets.Issue number, if available:
Changes
If stack is passed, we correctly convert to
BackendIdentifierbefore getting secrets.Corresponding docs PR, if applicable:
Validation
Updated test.
Checklist
run-e2elabel set.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.