Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(core): write Metadata resource in core framework #10306
fix(core): write Metadata resource in core framework #10306
Changes from 10 commits
8dea0fe
250fecb
e8d1673
2986948
d34489e
8bc1979
c62f0bc
80773c8
906b927
30ae434
b815752
96c1879
1eadb6d
6c0119c
fc7229d
183026f
71b8536
a3fc956
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should this be a property of Stack such as
stack.versionReportingEnabled
which will take into account context and the nested stack posture instead of leaking the fact that this can he set through context?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 was also thinking that this should be an opt-in switch ("enable") and not opt-out ("disable"). This way, we (and our users) won't need to fix all their unit tests because the metadata resource will only be attached if the CLI sets that context flag.
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.
We could do that, but since we already have an existing opt-out flag:
DISABLE_VERSION_REPORTING
(which we need to support for backwards compatibility anyway) I didn't really want to add anotherENABLE_VERSION_REPORTING
flag as well.The
Stack
property probably makes sense although it would need to be exposed as apublic
property or a (ew!)public @internal
one so that this function external to the class can use it.It fights with the App's
{ runtimeInfo?: @default true }
property as well...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.
DISABLE_VERSION_REPORTING
is not really public API, so I would recommend removing it in favor ofENABLE_VERSION_REPORTING
and deprecateruntimeInfo
in favor ofversionReporting
(in props and at theStack
level as public API.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.
Why?
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 believe I described that twice.
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.
Won't it make sense to just not include the CLI version in case this environment variable in not defined?
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's not actually about the
CLI_VERSION_ENV
itself. I'm usingCLI_VERSION_ENV
as a trigger to determine whether we're running under the CLI (and hence emitting the metadata)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.
Make 'CDKMetadata' a const