-
Notifications
You must be signed in to change notification settings - Fork 117
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
ConfigGroup V2 (Component) #2844
Conversation
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.
A bunch of minor comments, but the overall direction looks good.
One meta-comment: I'd encourage you to write an extended commit message that includes notes about the motivation for this change and explains any nonobvious implementation details. This is a major change that will be scrutinized by maintainers in the future who may not have the full context. Your future self will thank you! :)
Does the PR have any schema changes?Looking good! No breaking changes found. New resources:
|
a10d723
to
3f91f45
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2844 +/- ##
==========================================
+ Coverage 25.46% 26.62% +1.16%
==========================================
Files 48 52 +4
Lines 7532 7688 +156
==========================================
+ Hits 1918 2047 +129
- Misses 5456 5471 +15
- Partials 158 170 +12 ☔ View full report in Codecov by Sentry. |
I assume they can't be supported? I suppose when we have support for Pulumi-wide transformations on MLCs, they will work for this as an alternative. But there are some scenarios that I believe the K8s-specific transforms support that Pulumi transforms don't? Do we have a summary of common transforms use cases, and commonality of transforms, to inform whether we're comfortable that users can/will adopt this?
Is this list exhaustive? My biggest questions/concern is around potential breaking changes and/or things that are going to make adoption difficult.
By "the object name", does this mean the component itself? Curious why we are aligning with the .NET implementation vs. the others? Not clear apriori that the resourcePrefix should apply to the component (where the user does already have control over the name). |
@lukehoban sorry for the misunderstanding, by "object name" I'm referring to the Kubernetes object name, and I think the .NET implementation had it "right" in doing that. The component name is NOT mangled. I'll clarify. This new implementation is being offered side-by-side with the existing implementation, it is not (yet) a drop-in replacement. I suppose the new implementation should be characterized as a preview, and will make some adjustments to that effect. It is true that we have no plan (yet) to provide for Kubernetes-style transformation. I had a brief discussion with the platform team, and it seems like it would necessitate a callback system. The general transformation system is apparently based on an internal callback system that could become the basis in future. |
fa95a4e
to
f28f9f1
Compare
Just trying to make sure I follow the full plan - is this right?
|
That makes sense - but I’m still really interested in how much impact this will have on the question of “can all v1 users adopt v2?” What are the things someone might be doing in v1, especially with transformations, that are simply not at all possible with v2? what are technically possible but very hard - and will require detailed migration guide to help users through the migration? |
f28f9f1
to
006dd0d
Compare
@lukehoban yes that's the plan. I would characterize this new effort as a "preview" until the next major release, when we'll GA the "v2" and kick off the deprecation of "v1". Regarding the bugs in the v1 implementations, I believe we intend to close many as "won't fix" without being too dogmatic. Transformations are the basis for many use cases, e.g.
Pulumi transformation support will help with (1) but might not make for a good experience for (2). Also, Pulumi YAML would assumedly not have transformation support of any kind. For these reasons, I would advocate for providing more built-in transformations (e.g. a To recap: all users will be able to move to the GA of "v2", if we have Pulumi transformations and we also deliver the other resource types. |
The
|
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.
One minor suggestion, but otherwise the code LGTM.
006dd0d
to
4388af3
Compare
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@pulumi/kubernetes](https://pulumi.com) ([source](https://togithub.com/pulumi/pulumi-kubernetes)) | dependencies | minor | [`4.9.1` -> `4.10.0`](https://renovatebot.com/diffs/npm/@pulumi%2fkubernetes/4.9.1/4.10.0) | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>pulumi/pulumi-kubernetes (@​pulumi/kubernetes)</summary> ### [`v4.10.0`](https://togithub.com/pulumi/pulumi-kubernetes/blob/HEAD/CHANGELOG.md#4100-April-11-2024) [Compare Source](https://togithub.com/pulumi/pulumi-kubernetes/compare/v4.9.1...v4.10.0) - ConfigGroup V2 ([https://github.com/pulumi/pulumi-kubernetes/pull/2844](https://togithub.com/pulumi/pulumi-kubernetes/pull/2844)) - ConfigFile V2 ([https://github.com/pulumi/pulumi-kubernetes/pull/2862](https://togithub.com/pulumi/pulumi-kubernetes/pull/2862)) - Bugfix for ambiguous kinds ([https://github.com/pulumi/pulumi-kubernetes/pull/2889](https://togithub.com/pulumi/pulumi-kubernetes/pull/2889)) - \[yaml/v2] Support for resource ordering [https://github.com/pulumi/pulumi-kubernetes/pull/2894](https://togithub.com/pulumi/pulumi-kubernetes/pull/2894)4) - Bugfix for deployment await logic not referencing the correct deployment status ([https://github.com/pulumi/pulumi-kubernetes/pull/2943](https://togithub.com/pulumi/pulumi-kubernetes/pull/2943)) ##### New Features A new MLC-based implementation of `ConfigGroup` and of `ConfigFile` is now available in the "yaml/v2" package. These resources are usable in all Pulumi languages, including Pulumi YAML and in the Java Pulumi SDK. Note that transformations aren't supported in this release (see [https://github.com/pulumi/pulumi/issues/12996](https://togithub.com/pulumi/pulumi/issues/12996)). </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yODcuMSIsInVwZGF0ZWRJblZlciI6IjM3LjI4Ny4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJyZW5vdmF0ZS9ucG0iLCJ0eXBlL21pbm9yIl19--> Co-authored-by: lumiere-bot[bot] <98047013+lumiere-bot[bot]@users.noreply.github.com>
Proposed changes
This PR implements a new
ConfigGroup
resource (v2) as a Pulumi multi-language component (MLC). The new resource is offered side-by-side with the existing (v1) resource; it is not a drop-in replacement at this time.Some notable differences with respect to the previous (overlay) implementations:
resourcePrefix
property to override. Note that theresourcePrefix
is not applied to the Kubernetes object names.Examples
Here's some usage examples.
YAML
NodeJS
Testing
a. Unit tests for the core parsing logic (here)
b. Unit tests for the ConfigGroup resource (here)
a. New tests for the
Construct
method (which delegates to a component provider) (here)Related issues (optional)
Closes #2784