-
Notifications
You must be signed in to change notification settings - Fork 84
Feat: Use sandbox id by default for generation commands #669
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
Changes from 6 commits
1f2ae8d
2964cb0
a49ed38
e32c2a8
0b304fc
3de1ae5
2dc8035
be7c8a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@aws-amplify/backend-cli': minor | ||
--- | ||
|
||
Defaults to the sandbox identifier when no branch or stack is passed in the CLI |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
import assert from 'node:assert'; | ||
import { it } from 'node:test'; | ||
import { SandboxBackendIdResolver } from '../commands/sandbox/sandbox_id_resolver.js'; | ||
import { BackendIdentifierResolver } from './backend_identifier_resolver.js'; | ||
import { BackendIdentifierResolverWithFallback } from './backend_identifier_with_sandbox_fallback.js'; | ||
|
||
void it('if backend identifier resolves without error, the resolved id is returned', async () => { | ||
const namespaceResolver = { | ||
resolve: () => Promise.resolve('testAppName'), | ||
}; | ||
|
||
const defaultResolver = new BackendIdentifierResolver(namespaceResolver); | ||
const sandboxResolver = new SandboxBackendIdResolver(namespaceResolver); | ||
const backendIdResolver = new BackendIdentifierResolverWithFallback( | ||
defaultResolver, | ||
sandboxResolver | ||
); | ||
const resolvedId = await backendIdResolver.resolve({ | ||
appId: 'hello', | ||
branch: 'world', | ||
}); | ||
assert.deepEqual(resolvedId, { | ||
namespace: 'hello', | ||
name: 'world', | ||
type: 'branch', | ||
}); | ||
}); | ||
|
||
void it('uses the sandbox id if the default identifier resolver fails', async () => { | ||
const appName = 'testAppName'; | ||
const namespaceResolver = { | ||
resolve: () => Promise.resolve(appName), | ||
}; | ||
|
||
const defaultResolver = new BackendIdentifierResolver(namespaceResolver); | ||
const username = 'test-user'; | ||
const sandboxResolver = new SandboxBackendIdResolver( | ||
namespaceResolver, | ||
() => | ||
({ | ||
username, | ||
} as never) | ||
); | ||
const backendIdResolver = new BackendIdentifierResolverWithFallback( | ||
defaultResolver, | ||
sandboxResolver | ||
); | ||
const resolvedId = await backendIdResolver.resolve({}); | ||
assert.deepEqual(resolvedId, { | ||
namespace: appName, | ||
type: 'sandbox', | ||
name: 'test-user', | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
import { SandboxBackendIdResolver } from '../commands/sandbox/sandbox_id_resolver.js'; | ||
import { BackendIdentifierResolver } from './backend_identifier_resolver.js'; | ||
|
||
/** | ||
* Resolves the backend id when branch or stack is passed as an arg, otherwise returns a sandbox backend identifier | ||
*/ | ||
export class BackendIdentifierResolverWithFallback { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this implement There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see: #669 (comment) |
||
/** | ||
* Accepts the sandbox id resolver as a parameter | ||
*/ | ||
constructor( | ||
private defaultResolver: BackendIdentifierResolver, | ||
private fallbackResolver: SandboxBackendIdResolver | ||
) {} | ||
/** | ||
* resolves the backend id, falling back to the sandbox id if there is an error | ||
*/ | ||
public resolve = async ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: public is the default |
||
...args: Parameters<BackendIdentifierResolver['resolve']> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pretty sure this type will be inferred from the implementing interface. If not, just type it with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yep now that this is implementing the interface, this should be removed. |
||
) => { | ||
return ( | ||
(await this.defaultResolver.resolve(...args)) ?? | ||
(await this.fallbackResolver.resolve()) | ||
); | ||
}; | ||
} |
Uh oh!
There was an error while loading. Please reload this page.
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.
Having
BackendIdentifierResolver
andBackendIdentityResolver
is confusing. It's difficult to know what the difference between each is. Could we have a singleBackendIdentifierResolver
type with two implementation: one that resolves a single type of BackendId and another that goes through the fallback chain?If that proves to be too much refactoring, I think I'd rather just add the sandbox fallback directly to the existing
BackendIdentifierResolver
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.
@edwardfoyle How do you feel about this? 2dc8035#diff-9db562c416fe5dbc758bce7b582f0b1c080d36a408d4e2b12c457f9bbc7645b3R20