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

Feature flags are being moved to cdk.context.json #7399

Closed
rix0rrr opened this issue Apr 17, 2020 · 6 comments · Fixed by #7563
Closed

Feature flags are being moved to cdk.context.json #7399

rix0rrr opened this issue Apr 17, 2020 · 6 comments · Fixed by #7563
Assignees
Labels
bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 17, 2020

From #6929

Another thing I'd like to understand is the difference between cdk.json and cdk.context.json. I noticed if I put things like "@aws-cdk/core:enableStackNameDuplicates": "true", in cdk.json once I run diff they are moved to cdk.context.json, not sure why.

This should not happen. Overloading the flags as context makes it weird...


This is 🐛 Bug Report

@rix0rrr rix0rrr added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 17, 2020
@shivlaks shivlaks added the p1 label Apr 17, 2020
@shivlaks shivlaks changed the title Future flags are being moved to cdk.context.json Feature flags are being moved to cdk.context.json Apr 17, 2020
@eladb
Copy link
Contributor

eladb commented Apr 18, 2020

I agree

@abelmokadem
Copy link
Contributor

Thanks for opening this issue. This behaviour is quite annoying when you are doing some tests deployments on your local machine. Since we are not using cdk.context.json for our CI/CD, I have to keep reverting the git changes before committing.

@shivlaks
Copy link
Contributor

@rix0rrr the issue is this method we have to migrate legacy context which has a heuristic that's fragile as it marks any context key with a : in it as eligible for moving. This logic was initially intended to move relevant context provider data into cdk.context.json

also @eladb called this one. It seems like we intended to remove the migration logic altogether a long time ago but did not. Is that still the desired outcome here?

If we need to keep the logic, we'd need to move to an explicit whitelist of keys that are eligible for moving. Otherwise, we're precluding usage (internal or user provided) of : character in context.

@shivlaks shivlaks added effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Apr 23, 2020
@eladb
Copy link
Contributor

eladb commented Apr 23, 2020

I don't like the migration logic. As a user, if I configure application context in cdk.json, I would expect it to stay there. I think cdk.context.json should be kept for context providers.

@shivlaks
Copy link
Contributor

@eladb agreed.

Is the use case for moving legacy context provider keys still a valid one? this change was introduced in 0.25.1. If not we can remove altogether.

@eladb
Copy link
Contributor

eladb commented Apr 23, 2020

@eladb agreed.

Is the use case for moving legacy context provider keys still a valid one? this change was introduced in 0.25.1. If not we can remove altogether.

I think we can remove

@mergify mergify bot closed this as completed in #7563 Apr 23, 2020
mergify bot pushed a commit that referenced this issue Apr 23, 2020
…ext.json

Removes migration logic that was introduced to move legacy context provider keys.

Rationale:
This logic was intended to be removed prior to v1.0
It looks for any key that contains a `:` and moves it to `cdk.context.json`. This is
not expected behavior and also prevents users from having keys that have `:`. Our
init templates also include feature flags which make use of the `:` character.

Closes #7399
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. effort/small Small work item – less than a day of effort p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants