🌱 Add ClusterClass variables to status on reconcile #7991

killianmuldoon · 2023-01-24T18:18:48Z

Add variables from the ClusterClass .spec to its .status field on each reconcile. This change is the foundation for introducing external variables which can be kept in sync in the ClusterClass status.

Part of #7985

api/v1beta1/clusterclass_types.go

killianmuldoon

/hold

This PR contains a couple of necessary commits from other PRs, so it should not be merged until they're also merged.

killianmuldoon · 2023-01-27T11:31:59Z

/remove-hold

sbueringer · 2023-01-30T11:18:57Z

@killianmuldoon Please rebase, when you have some time

This PR contains a couple of necessary commits from other PRs, so it should not be merged until they're also merged.

I assume this is not the case anymore?

api/v1beta1/clusterclass_types.go

internal/controllers/clusterclass/clusterclass_controller.go

killianmuldoon · 2023-01-30T14:59:07Z

This PR contains a couple of necessary commits from other PRs, so it should not be merged until they're also merged.

I assume this is not the case anymore?

Yeah - those PRs are now merged.

killianmuldoon · 2023-01-30T14:59:37Z

/remove-hold

Underlying PRs are now merged.

killianmuldoon · 2023-01-30T18:10:36Z

docs/book/src/tasks/experimental-features/runtime-sdk/implement-topology-mutation-hook.md

@@ -86,7 +86,7 @@ status:
    variables:


@vincepri would really appreciate if you have the time to review the API, particularly how the variables appear in ClusterClass .status and how they can be defined by users in Cluster .spec.topology.

We had a discussion about it - with @sbueringer, @fabriziopandini, @ykakarap - but it would be good to get another set of eyes 🙂

+1 to the current naming from my side as discussed

sbueringer · 2023-01-30T18:28:27Z

/retest

ykakarap

Just nits. But lets wait to address them after the API is finalized.

api/v1beta1/clusterclass_types.go

docs/book/src/tasks/experimental-features/runtime-sdk/implement-topology-mutation-hook.md

internal/controllers/clusterclass/clusterclass_controller_test.go

sbueringer · 2023-01-31T12:54:12Z

/retest

rate limit flake

sbueringer · 2023-01-31T13:09:05Z

/lgtm

k8s-ci-robot · 2023-01-31T13:09:11Z

LGTM label has been added.

Git tree hash: bd1204d15693e08725836e43f2424bbc1ad98b21

vincepri · 2023-01-31T14:23:18Z

api/v1beta1/clusterclass_types.go

+	// DefintionsConflict specifies whether or not there are conflicting definitions for a single variable name.
+	// +optional
+	DefintionsConflict bool `json:"defintionsConflict,omitempty"`


Suggested change

// DefintionsConflict specifies whether or not there are conflicting definitions for a single variable name.

// +optional

DefintionsConflict bool `json:"defintionsConflict,omitempty"`

// Conflicts specifies whether or not there are conflicting definitions for a single variable name.

// +optional

Conflicts bool `json:"conflicts,omitempty"`

Maybe?

Not sure if the additional Definitions is better. Essentially it's variables[].definitionsConflict vs variables[].conflicts

The additional hint that the definitions conflict and not the variable itself might be more intuitive

+1 I also prefer definitionsConflict here (assuming we're sticking with definition)

I'm slightly in favor of .conflict, it is simpler and in this struct we only have definitions, so...
But I will be ok with both.

api/v1beta1/clusterclass_types.go

vincepri · 2023-01-31T14:29:33Z

docs/book/src/tasks/experimental-features/runtime-sdk/implement-topology-mutation-hook.md

        - name: http-proxy
-          namespace: inline
+          definitionFrom: inline


Are there alternative names to this variable? Don't have better ideas right now, but not sure if it's immediately clear that this is where the schema of the variable is defined

I guess schemaFrom could be an alternative. We just used definitions in the ClusterClass.status so it's nice if that matches up. We used definitions there as it felt consistent to patches[].definitions.

But we could change both places to ClusterClass.status.variables[].schemas and Cluster.spec.topology.variables[].schemaFrom

But I think I prefer definition / definitionFrom over schema / schemaFrom as we also have schema one level deeper already to describe the actual VariableSchema (which doesn't include if the variable itself is required). So it's confusing if we have it on both levels.

To summarize. Variable definition for us is:

the name of the variable

the VariableSchema

if the variable itself is required

Based on that it seems fitting to go with definition rather than schema

I think we should be consistent with whatever we go for here - but in the existing API Schema is below Definition as Stefan said, open to other options for the word though.

I'm +1 with definition, it is a consistent with what we have in clusterClass

sbueringer · 2023-01-31T17:29:40Z

/lgtm

k8s-ci-robot · 2023-01-31T17:29:44Z

LGTM label has been added.

Git tree hash: a2c2c987c02eb3163347cb126bb0badb410b907e

fabriziopandini · 2023-01-31T21:01:02Z

api/v1beta1/common_types.go

+
+	// VariableDefinitionFromInline indicates a patch or variable was defined in the `.spec` of a ClusterClass
+	// rather than from an external patch extension.
+	VariableDefinitionFromInline = "inline"


wondering if inline is clear enough for users looking at the cluster, but I don't have better suggestions 😓

I'm not sure if there are better options, given that we define variables that are defined in the ClusterClass as "inline variables" everywhere

The other option IMO is to be completely literal and point to 'ClusterClass .spec.variables'. I think this assumes too much knowledge of the ClusterClass to Cluster authors.

Cluster authors should really see and treat this as meaningless name for setting variables. For ClusterClass authors they should know what inline patches and variables.

fabriziopandini · 2023-02-01T10:47:14Z

Let's merge this, so we unblock other work.
We can iterate on those names later on if necessary; please open an issue/add a point in your tracking issue about doing a final UX audit of this feature later in the cyle with links to the above threads
/lgtm
/approve

k8s-ci-robot · 2023-02-01T10:47:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [fabriziopandini]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 24, 2023

k8s-ci-robot requested review from enxebre and stmcginnis January 24, 2023 18:19

killianmuldoon commented Jan 24, 2023

View reviewed changes

api/v1beta1/clusterclass_types.go Outdated Show resolved Hide resolved

killianmuldoon mentioned this pull request Jan 24, 2023

External variable discovery #7985

Closed

34 tasks

killianmuldoon commented Jan 24, 2023

View reviewed changes

api/v1beta1/clusterclass_types.go Outdated Show resolved Hide resolved

killianmuldoon commented Jan 26, 2023

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 26, 2023

killianmuldoon changed the title ~~🌱 [WIP] Add ClusterClass variables to status on reconcile~~ 🌱 Add ClusterClass variables to status on reconcile Jan 27, 2023

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 27, 2023

sbueringer reviewed Jan 30, 2023

View reviewed changes

api/v1beta1/clusterclass_types.go Outdated Show resolved Hide resolved

internal/controllers/clusterclass/clusterclass_controller.go Show resolved Hide resolved

killianmuldoon force-pushed the pr-exvars-cc-status-var branch from c232f78 to 2d21ae8 Compare January 30, 2023 14:57

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2023

killianmuldoon force-pushed the pr-exvars-cc-status-var branch from 2d21ae8 to f8ac252 Compare January 30, 2023 18:07

killianmuldoon commented Jan 30, 2023

View reviewed changes

ykakarap reviewed Jan 31, 2023

View reviewed changes

api/v1beta1/clusterclass_types.go Outdated Show resolved Hide resolved

api/v1beta1/clusterclass_types.go Outdated Show resolved Hide resolved

killianmuldoon force-pushed the pr-exvars-cc-status-var branch 2 times, most recently from 6a0737a to 15c89dd Compare January 31, 2023 10:34

sbueringer reviewed Jan 31, 2023

View reviewed changes

killianmuldoon force-pushed the pr-exvars-cc-status-var branch from 15c89dd to 4d093c7 Compare January 31, 2023 13:02

k8s-ci-robot assigned sbueringer Jan 31, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2023

vincepri reviewed Jan 31, 2023

View reviewed changes

killianmuldoon force-pushed the pr-exvars-cc-status-var branch from 4d093c7 to f5068cf Compare January 31, 2023 16:01

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2023

k8s-ci-robot requested a review from sbueringer January 31, 2023 16:01

Add ClusterClass variables to status on reconcile

0118e20

killianmuldoon force-pushed the pr-exvars-cc-status-var branch from f5068cf to 0118e20 Compare January 31, 2023 16:29

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2023

fabriziopandini reviewed Jan 31, 2023

View reviewed changes

k8s-ci-robot assigned fabriziopandini Feb 1, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2023

k8s-ci-robot merged commit e9a7318 into kubernetes-sigs:main Feb 1, 2023

k8s-ci-robot added this to the v1.4 milestone Feb 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Add ClusterClass variables to status on reconcile #7991

🌱 Add ClusterClass variables to status on reconcile #7991

killianmuldoon commented Jan 24, 2023

killianmuldoon left a comment

killianmuldoon commented Jan 27, 2023

sbueringer commented Jan 30, 2023 •

edited

Loading

killianmuldoon commented Jan 30, 2023

killianmuldoon commented Jan 30, 2023

killianmuldoon Jan 30, 2023

sbueringer Jan 31, 2023

sbueringer commented Jan 30, 2023

ykakarap left a comment

sbueringer commented Jan 31, 2023

sbueringer commented Jan 31, 2023

k8s-ci-robot commented Jan 31, 2023

vincepri Jan 31, 2023

sbueringer Jan 31, 2023

killianmuldoon Jan 31, 2023

fabriziopandini Jan 31, 2023

vincepri Jan 31, 2023

sbueringer Jan 31, 2023

sbueringer Jan 31, 2023 •

edited

Loading

killianmuldoon Jan 31, 2023

fabriziopandini Jan 31, 2023

sbueringer commented Jan 31, 2023

k8s-ci-robot commented Jan 31, 2023

fabriziopandini Jan 31, 2023

sbueringer Feb 1, 2023

killianmuldoon Feb 1, 2023

fabriziopandini commented Feb 1, 2023

k8s-ci-robot commented Feb 1, 2023

🌱 Add ClusterClass variables to status on reconcile #7991

🌱 Add ClusterClass variables to status on reconcile #7991

Conversation

killianmuldoon commented Jan 24, 2023

killianmuldoon left a comment

Choose a reason for hiding this comment

killianmuldoon commented Jan 27, 2023

sbueringer commented Jan 30, 2023 • edited Loading

killianmuldoon commented Jan 30, 2023

killianmuldoon commented Jan 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer commented Jan 30, 2023

ykakarap left a comment

Choose a reason for hiding this comment

sbueringer commented Jan 31, 2023

sbueringer commented Jan 31, 2023

k8s-ci-robot commented Jan 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer commented Jan 31, 2023

k8s-ci-robot commented Jan 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini commented Feb 1, 2023

k8s-ci-robot commented Feb 1, 2023

sbueringer commented Jan 30, 2023 •

edited

Loading

sbueringer Jan 31, 2023 •

edited

Loading