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

[core] Inconsistent context type(s) #8865

Closed
fogfish opened this issue Jul 2, 2020 · 1 comment · Fixed by #9014
Closed

[core] Inconsistent context type(s) #8865

fogfish opened this issue Jul 2, 2020 · 1 comment · Fixed by #9014
Assignees
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md in-progress This issue is being actively worked on. p2

Comments

@fogfish
Copy link
Contributor

fogfish commented Jul 2, 2020

The CDK context is defined as a pair string -> any

ConstructNode.setContext(key: string, value: any): void
ConstructNode.tryGetContext(key: string): any

However AppProps defines a context as string -> string pair.

export interface AppProps {
  ...
  readonly context?: {
        [key: string]: string;
    };
}

Reproduction Steps

Once an application start to use JSON structs as a context value then it becomes almost impossible to mock an application with the context (everything works with deployment because context is passed via cdk.json):

const app = new cdk.App({ context: { /* do not allow to pass anything then string -> string pairs */  }  })
app.node.setContext() // fails with error message "unable to modify a tree (or something like that)

Here two major question:

  1. Is the usage of JSON structs is valid in the context? I believe yes, because of Vpc.fromLookup does it.
  2. Should we change the cdk.App to accept string -> any pair so that we can mock the context

Environment

  • CLI Version : 1.47.1 (build 8bb7c0d)
  • Framework Version:
  • Node.js Version: v12.12.0
  • OS : Mac
  • **Language (Version):**TypeScript (3.8.3)
@fogfish fogfish added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jul 2, 2020
@github-actions github-actions bot added the @aws-cdk/core Related to core CDK functionality label Jul 2, 2020
@eladb
Copy link
Contributor

eladb commented Jul 8, 2020

Let's change to any. @fogfish would you like to submit a PR?

@eladb eladb added effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p2 labels Jul 8, 2020
@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label Jul 8, 2020
@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Jul 10, 2020
@mergify mergify bot closed this as completed in #9014 Jul 16, 2020
mergify bot pushed a commit that referenced this issue Jul 16, 2020
curtiseppel pushed a commit to curtiseppel/aws-cdk that referenced this issue Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md in-progress This issue is being actively worked on. p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants