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

Make ApiObject.props public #2236

Open
2 tasks
erikschul opened this issue Oct 30, 2024 · 2 comments
Open
2 tasks

Make ApiObject.props public #2236

erikschul opened this issue Oct 30, 2024 · 2 comments
Labels
@component/cdk8s-cli Issue related to cdk8s-cli effort/medium 1 week tops feature-request New/Enhanced functionality wanted priority/p2 Dependent on community feedback. PR's are welcome :)

Comments

@erikschul
Copy link

erikschul commented Oct 30, 2024

Description of the feature or enhancement:

I would like to use cdk8s-plus types, but after they've been created (e.g. new KubeDeployment(...)), the props are not accessible, so KubeDeployment (etc.) cannot be injected as a dependency, which I thought is the normal practice in CDK, e.g. new MyCustomServiceType(scope, id, { deployment: myDeployment })

Is it possible to change this to public? If no, why not? And in that case, is it technically possible to fork the repo and change it, or are the technical reasons why this cannot be made public? I understand that people may try to modify properties, but it is readonly.

export declare class ApiObject extends Construct {
    private readonly props;

Use Case:

Proposed Solution:

Other:

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@erikschul erikschul added feature-request New/Enhanced functionality wanted needs-triage Priority and effort undetermined yet labels Oct 30, 2024
@iliapolo
Copy link
Member

iliapolo commented Dec 9, 2024

Its slightly more involved than just turning private to public because input values could be tokens that only get resolved during synthesis. So a caller of myDeployment.myProp would have to include Token checks throughout the code. It is possible though.

Another ergonomic issue is that myDeployment.Prop doesn't really map to the k8s mental model. If any, it should be myDeployment.spec.Prop, but not all objects have .spec (some have .data for example).

I think what we need is for our L1 code generator to put those public spec or data properties on the individual constructs, and not on the ApiObject one.

In any case, as a workaround for now, you could do:

const spec = myDeployment.toJson().spec;
....

@iliapolo iliapolo added effort/medium 1 week tops priority/p2 Dependent on community feedback. PR's are welcome :) @component/cdk8s-cli Issue related to cdk8s-cli and removed needs-triage Priority and effort undetermined yet labels Dec 9, 2024
@erikschul
Copy link
Author

The issue with .toJson().spec; is that it loses typing, which for me was the main reason for using CDK8s :)

I agree that using .props is unidiomatic, but it's a one-liner. I agree about the potential for erroneous use, but perhaps such an error can be caught with try-catch and an appropriate error message can guide the user? I.e. using a property proxy.

But if the generator can be updated, that would be even better. Would that also support imported CRDs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@component/cdk8s-cli Issue related to cdk8s-cli effort/medium 1 week tops feature-request New/Enhanced functionality wanted priority/p2 Dependent on community feedback. PR's are welcome :)
Projects
None yet
Development

No branches or pull requests

2 participants