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

Incorporate CRDs into Python codebase #66

Open
benglewis opened this issue May 28, 2024 · 8 comments
Open

Incorporate CRDs into Python codebase #66

benglewis opened this issue May 28, 2024 · 8 comments
Labels
kind/enhancement Improvements or new features

Comments

@benglewis
Copy link

Hello!

  • Vote on this issue by adding a 👍 reaction
  • If you want to implement this feature, comment to let us know (we'll work with you on design, scheduling, etc.)

Issue details

Use crd2pulumi with slight adjustments to create Pulumi Python library code for Custom Resource Definitions. As of now, I am doing this in my own codebase to do the nice type-checking and write Python code rather than the unchecked CRD shown in the example, but it would be nice to have it be official. I can contribute my changes to the output from crd2pulumi if it would help.

Affected area/feature

Pulumi integration with cert-manager CRDs

@benglewis benglewis added kind/enhancement Improvements or new features needs-triage Needs attention from the triage team labels May 28, 2024
@blampe blampe added the help wanted Extra attention is needed label May 28, 2024
@blampe
Copy link
Contributor

blampe commented May 28, 2024

Use crd2pulumi with slight adjustments to create Pulumi Python library code for Custom Resource Definitions. As of now, I am doing this in my own codebase to do the nice type-checking and write Python code rather than the unchecked CRD shown in the example, but it would be nice to have it be official.

@benglewis could you give me an example of the code you're generating, and how that differs from the published Python types?

Also, are you referring to this example or something else? I'm unsure what you mean by unchecked, what would you expect it to look like instead?

I can contribute my changes to the output from crd2pulumi if it would help.

Help is always welcome and appreciated!

@blampe blampe added awaiting-feedback Blocked on input from the author and removed needs-triage Needs attention from the triage team labels May 28, 2024
@benglewis
Copy link
Author

@blampe https://github.com/benglewis/pulumi-kubernetes-cert-manager
Please take a look at my fork :)
I even opened a PR at #71

@benglewis
Copy link
Author

A little more context, essentially all I did was run https://github.com/pulumi/crd2pulumi on the CRDs YAML that cert-manager publish with releases on their GitHub (e.g. cert-manager.crds.yaml ), and I made some minor adjustments to the output Python to fix a bug in the crd2pulumi codegen (pulumi/crd2pulumi#50) and to ensure that the code is accessible under a second package (for separation of concerns) and then I updated the example. Hopefully everything should work nicely with this :)

@blampe
Copy link
Contributor

blampe commented May 30, 2024

@benglewis thank you for putting this together!

Some thoughts/questions:

  • It would probably be beneficial to do something similar for the other languages, no?
  • Backwards compatibility is very important; people using existing types shouldn't see their programs fail to compile when they update the package. That probably means we would want to generate these types under a new import path.
  • We would need this to be easily reproducible, ideally running crd2pulumi as part of the build target without needing manual intervention.
  • It's unfortunate you ran into Python code imports do not work crd2pulumi#50 -- there are a few similar codegen issues that crd2pulumi suffers from. Could that be worked around with some sed commands or similar?

@benglewis
Copy link
Author

  • Yup @blampe , it definitely would be, but since I don't use Pulumi in another language, I don't know how to verify the results of crd2pulumi for those languages (since the things that I manually fixed will probably occur in other languages too)
  • I did so, the CRD types are in a separate package. I don't think that this will break anything.
  • I entirely agree that it would be best to use crd2pulumi in the CI. I can look at that, but since some manual changes were needed, I will need to convert those to standard BASH commands using rm and sed, yes
  • Definitely.

I saw that you tagged @EronWright in my PR for review. If you can confirm that if I replace the manually run of crd2pulumi to an automated CI call with rm and sed calls, then the PR can be merged, then I will invest the time in doing it; I just don't want to go to that effort if it isn't clear that this change will be upstreamed :)

@mikhailshilkov mikhailshilkov added needs-triage Needs attention from the triage team and removed awaiting-feedback Blocked on input from the author labels Jun 5, 2024
@mjeffryes
Copy link
Member

@benglewis Conceptually, this is a great idea; we'd love to have typed APIs for lots of charts in the registry. However, I don't think we can take take exactly the approach from your PR.

There's some practical concerns like the ones @blampe mentioned above, but we're also currently making an investment in an overhaul of crd2pulumi that should fix lots of the current SDK generation issues so that using that tool directly in your projects will become much more practical.

I also recognize that even if crd2pulumi is perfect, it would still be nice to get those SDKs into our registry somehow so they can have documentation, be published in to PyPi and other language package managers, etc. We have some ideas for how to do that in a way that can scale to many CRDs. In the meantime, we can keep this ticket open to track the need (and collect upvotes here).

@mjeffryes mjeffryes removed needs-triage Needs attention from the triage team help wanted Extra attention is needed labels Jun 11, 2024
@EronWright
Copy link
Contributor

I do love the idea of providing strongly-typed wrappers for the CRDs of cert-manager, whether generated by crd2pulumi or by another tool. Obviously the wrappers should work seamlessly with this @pulumi/kubernetes-cert-manager component. Sorry for the wait as we figure out some details.

@silasdavis
Copy link

silasdavis commented Jul 1, 2024

Can we widen this issue to include other languages not just Python? It's a particular shame the typescript SDK does not include these given that:

a) crd2pulumi exists and explicitly calls out cert-manager as part of it raison d'être
(https://github.com/pulumi/crd2pulumi?tab=readme-ov-file#goals)!
b) The work for typescript is substantially already done in this other Pulumi-owned repo: https://github.com/pulumi/cert-manager-examples/tree/master/cert-manager/crds/nodejs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements or new features
Projects
None yet
Development

No branches or pull requests

6 participants