-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
fix(geo): update amplify config geo schema #13290
fix(geo): update amplify config geo schema #13290
Conversation
@@ -51,7 +51,7 @@ export interface AmplifyOutputsStorageProperties { | |||
export interface AmplifyOutputsGeoProperties { | |||
aws_region: string; | |||
maps?: { | |||
items: Record<string, { name: string; style: string }>; | |||
items: Record<string, unknown>; |
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.
Option 1(less strict): keep type same as resource config. Similar to other categories
maps?: {
items: Record<string, unknown>;
default: string;
};
Option 2(more strict): remove name keep existing structure
maps?: {
items: Record<string, { style: string }>;
default: string;
};
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.
Going with option1, please feel free to comment otherwise
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.
Is there a reason not to go with option 2? Seems like the cleaner solution
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.
Is there a reason we would want to do that? So the actual resource config itself is less stricter than what we had in outputsConfig and wondering if it would make sense to keep the same (consistency) since this is compiled down to ResourceConfig
at the end.
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.
Thanks @ashwinkumar6
Description of changes
below is the ref for backend schema that generates geo amplifyOutputs
Geo.map.items (
amazon_location_service_config
from the config above)amplify_outputs.json
Description of how you validated changes
Validated this change works with the geo gen2 cdk construct mentioned in docs
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.