Skip to content
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

programmatic generate client config starter #43

Merged
merged 33 commits into from
Jun 15, 2023
Merged

programmatic generate client config starter #43

merged 33 commits into from
Jun 15, 2023

Conversation

edwardfoyle
Copy link
Contributor

@edwardfoyle edwardfoyle commented Jun 10, 2023

Issue #, if available:
fixes #40

Description of changes:
Needs tests but it's ready for eyes on the overall approach

Starting point for fetching stack output and reconstructing into objects that can be passed to the construct plugins for parsing to frontend config. Only the output fetching and reconstruction is implemented here.

I refactored the ProjectEnvironmentIdentifier class into a new primitives package. This allows both the reader and writer of the SSM stack pointer to use the same ProjectEnvironmentIdentifier.toOutputStackSSMParameterName when resolving the ssm key name

I also added a zod schema for the metadata object that is stored in the stack. This allows the writer and reader to validate against the schema

The generate package introduces several new classes:

  1. ClientConfigGeneratorFactory produces ClientConfigGenerators given either a stack name or a project environment identifier
  2. DefaultClientConfigGenerator is largely a placeholder right now for when the actual frontend translation logic is implemented
  3. StackMetadataOutputRetrievalStrategy is the counterpart to StackMetadataOutputStorageStrategy that is implemented in backend-engine. It uses the shared schemas from the primitives package to load and validate the stack metadata and output
  4. ProjectEnvironmentStackNameResolver looks up the stack name based on the SSM param for the project environment
  5. PassThroughStackNameResolver returns the given stack name in the case where the stack name is already known (customer is using CDK directly)

Some open questions:

  1. Now that I've implemented this, I've gotten a little more clarity on this issue. I think we might want a BackendIdentificationStrategy in addition to the OutputStorageStrategy. The BackendIdentificationStrategy should perhaps go in the primitives package so that backend-engine can call something like backendIdStrat.setIdentifier(stack) and frontend generation can call backendIdStrat.resolveStackName()
  2. should the entry point to generateClientConfig be a function or an exported class that consumer constructs? How much DI should the consumer do vs the exported function package?

EDIT:
Another idea is instead of a "primitives" package (which could become a dumping ground for "stuff that needs to be shared between different packages"), introduce a "backend-metadata" package that is responsible for reading/writing metadata for a backend. The backend-engine can use this package for writing and the generate package can use it for reading. Essentially the "backend-metadata" package becomes the wrapper around our data layer (which is CFN metadata and outputs)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

packages/backend-engine/src/amplify_stack.ts Outdated Show resolved Hide resolved
packages/backend-engine/API.md Outdated Show resolved Hide resolved
packages/generate/API.md Outdated Show resolved Hide resolved
packages/generate/src/amplify_backend_output.ts Outdated Show resolved Hide resolved
packages/generate/src/client_config_generator.ts Outdated Show resolved Hide resolved
packages/generate/src/output_retrieval_strategy.ts Outdated Show resolved Hide resolved
Comment on lines 20 to 24
/**
* Gets Amplify backend outputs from stack metadata and outputs
*/
export class StackMetadataOutputRetrievalStrategy
implements OutputRetrievalStrategy
Copy link
Member

Choose a reason for hiding this comment

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

this type and factory should either go to backend-engine or "that long name for package that hosts metadata strategy logic"

@edwardfoyle
Copy link
Contributor Author

Apart from still needing more tests, this is ready for another review.

What changed:

  1. the primitives package and generate package have been removed. The stuff that was in primitives has been split into putting the types in plugin-types and putting the impl in backend-engine. The stuff that was in generate is now in backend-engine. This was done to prevent premature splitting into a bunch of tiny packages. I have created some README files in backend-engine to explain the current structure and hopefully provide some clarity to a future soul that might have to pull these parts into separate packages
  2. A BackendIdentifier has been introduced that decouples the customer facing backend identification method from the classes that have internal methods hanging off of them. The ProjectEnvironmentIdentifier is now a customer facing type with no internal details on it. Instead, the ProjectEnvironmentIdentifier can be passed into a BackendStackCreator or BackendStackResolver depending on the use case

* This key is shared between stack creation in \@aws-amplify/backend and stack discovery during client config generation.
* It is the "breadcrumb" that enables client config generation to discover the stack outputs for a given project environment
*/
export const getProjectEnvironmentMainStackSSMParameterKey = (
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to expose SSM in the name of this method as it's just an internal detail. It could just be getMainStackIdentifierKey or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather keep this one as-is. It should be super clear this key is for something very specific, ie using SSM to identify a stack name given a project environment. It shouldn't be used for anything else

packages/backend/src/default_stack_factory.ts Show resolved Hide resolved
packages/backend-engine/package.json Show resolved Hide resolved
packages/backend-engine/src/index.ts Outdated Show resolved Hide resolved
packages/plugin-types/API.md Show resolved Hide resolved
packages/plugin-types/API.md Outdated Show resolved Hide resolved
packages/plugin-types/API.md Outdated Show resolved Hide resolved
Comment on lines 25 to 26
// @public
export const getProjectEnvironmentMainStackSSMParameterKey: (projectEnvironmentIdentifier: ProjectEnvironmentIdentifier) => string;
Copy link
Member

Choose a reason for hiding this comment

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

After giving it more thoughts I think we should think about duplicating this logic in both packages and perhaps enforce that both implementations stay in sync in other ways.
Reason is - how do we roll major version bump if the identifier convention changes in a breaking way.

If we keep it here I suggest to wrap it in namespace since this is kind of leaking implementation details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Duplicating is probably okay here as this is something that will likely never change / would be a huge breaking change if we did

.eslintrc.json Outdated Show resolved Hide resolved
sobolk
sobolk previously approved these changes Jun 14, 2023
Copy link
Member

@sobolk sobolk left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@Amplifiyer Amplifiyer left a comment

Choose a reason for hiding this comment

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

Amazing work, thanks @edwardfoyle
:shipit:

@edwardfoyle edwardfoyle merged commit 0703074 into main Jun 15, 2023
@edwardfoyle edwardfoyle deleted the generate branch June 15, 2023 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reconsider how stack SSM parameter is generated
3 participants