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
feat: add api id and amplify environment name to stash #2273
feat: add api id and amplify environment name to stash #2273
Changes from all commits
2d1aa93
263efd0
59102df
304d3c1
bc9b37c
9c6ab19
80684b5
9ad666c
9aff173
e0d795c
e50269c
ec2c184
389b1a6
cd166ed
a5ca531
8987f6e
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.
This context var
amplifyEnvironmentName
looks like is set by data construct exclusively? Is this thebranchName
for CI/CD deployments andNONE
for sandbox?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.
Yes, this is this the branchName for CI/CD deployments and NONE for sandbox.
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.
than we are using the wrong naming here.
amplifyEnvironmentName
suggests amplify wide name for the environment which is not the case here. May I suggestamplifyApiEnvironmentName
?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.
The value is already used in the data construct. To use the new name without a breaking change we would need to set both variables, then it might be confusing why there are two variables with the same value. I think it is a better option to continue using the existing one.
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'm not asking to change in the data construct but rather the new variables created in the
backend-data
to which this value is assigned. As well as the new variable set in theconvert_js_resolvers.ts
. It's not a branch name since it's applicable for sandboxes 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.
Oh sorry, you mean
const amplifyEnvironmentName
not what is in 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.
yes, I understand context vars are harder to change, but we should not create more vars with this naming (such as
amplifyBranchName
)