- 
                Notifications
    You must be signed in to change notification settings 
- Fork 89
fix: add missing aws_iam directive on custom types #3270
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
Conversation
| E2E's and PR workflow are still failing in early stages. This has notably been occurring on prior PR's as well and warrants some investigation, which I'll perform prior to continuing with this PR. | 
| addIamAuthToCustomTypes: (ctx: TransformerTransformSchemaStepContextProvider) => void; | ||
| // (undocumented) | 
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.
It seems this could be private.
(on the other hand, we never paid a lot of attention to transformers public apis...).
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.
Yeah. I haven't formed a strong opinion here yet. I'm just following the established pattern for now. 🤷
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 would recommend adopting "if something can be private it should be". that creates less liability going forward. (we've been doing this for a while on backend side).
For transformers it would be more to form a habit (or maybe stop the bleeding) rather than fix them. Their apis are already overexposed.
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 might not be customer facing API. But once exposed and called by some other package on it becomes liability.
An example where versioning "internal apis" matter is here aws-amplify/amplify-backend#2717 (comment) .
(this might not be that important for transformers because api construct locks/bundles them afaik - yet another assumption in the system to excuse ourselves from api versioning...).
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.
Their apis are already overexposed
This might be a sign that all these transformers packages could have been directories in single package.
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 might agree. And, it all seems pretty chaotic to me at the moment. But, there's also a ton of intrinsic complexity in the problem space. So, I'm attempting to respect what came before and keep as many existing fences erected until I understand them better and have more solid opinions.
Description of changes
Adds
@aws_iamdirective to non-model types to support IAM auth from functions and other AWS services.CDK / CloudFormation Parameters Changed
Issue #, if available
Description of how you validated changes
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.