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

feat: adding kind.cluster.creation.image configuration property for kind cluster #3508

Merged
merged 8 commits into from Aug 14, 2023

Conversation

axel7083
Copy link
Contributor

What does this PR do?

The node's container image can be used to specify the Kubernetes version used for the control-planed. For example using kindest/node:v1.25.8 will create a cluster with Kubernetes 1.25.8.

Screenshot/screencast of this PR

Default creation page

image

Custom image creation page

image

Custom image without sha256

As explained in the official kind documentation

You can set a specific Kubernetes version by setting the node’s container image. You can find available image tags on the releases page. Please include the @sha256: image digest from the image in the release notes

image

What issues does this PR fix or reference?

Allows to create Kubernetes cluster with specific version.

How to test this PR?

  • [x]: added and changed unit tests for validating the changes.
  • [x]: tested to created a cluster with kindest/node:v1.27.2

@axel7083 axel7083 requested review from benoitf and a team as code owners August 11, 2023 15:20
@axel7083 axel7083 requested review from dgolovin and cdrage and removed request for a team August 11, 2023 15:20
@axel7083 axel7083 force-pushed the feature/kind-custom-image-support branch from 700076c to 8c063be Compare August 11, 2023 15:22
@benoitf
Copy link
Collaborator

benoitf commented Aug 11, 2023

hello @axel7083 and thanks for the contribution

I'm wondering if instead of having to specify the image we could not specify the version with a dropdown ?

like 1.21, 1.22, ... 1.27 and then it's the extension that picks up the right image ?

@axel7083
Copy link
Contributor Author

hello @axel7083 and thanks for the contribution

I'm wondering if instead of having to specify the image we could not specify the version with a dropdown ?

like 1.21, 1.22, ... 1.27 and then it's the extension that picks up the right image ?

I was hesitant to do that instead, but maybe some people would want to use custom image, and making a dropdown force use to ask which version do we support ? How far do we go under the minor version ? Some version go from X.X.0 to X.X.11, that would be very long dropdown. If we select only major version, people could desire to have more control over the version used.

extensions/kind/src/templates/create-cluster-conf.mustache Outdated Show resolved Hide resolved
extensions/kind/src/create-cluster.ts Outdated Show resolved Hide resolved
extensions/kind/src/create-cluster.ts Outdated Show resolved Hide resolved
extensions/kind/package.json Outdated Show resolved Hide resolved
extensions/kind/src/create-cluster.ts Outdated Show resolved Hide resolved
extensions/kind/src/create-cluster.ts Outdated Show resolved Hide resolved
@benoitf
Copy link
Collaborator

benoitf commented Aug 11, 2023

I was hesitant to do that instead, but maybe some people would want to use custom image, and making a dropdown force use to ask which version do we support ? How far do we go under the minor version ? Some version go from X.X.0 to X.X.11, that would be very long dropdown. If we select only major version, people could desire to have more control over the version used.

Yes there is simplicity over configurability here.

maybe 2 fields like one for the lazy people and another for the 'custom' value

one dropdown with latest, then some recent major versions like 1.27, 1.26 and then you have custom where it'll use the field that you're using

it's not blocking the PR, just that I think from usability, just selecting 1.26 or 1.27 is easier than going to an external site, looking for a string, copy pasting and coming back to enter the value.

Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the awesome contrib, if you could take some remarks (specific field for mustache template rather than extraConfig, all parameters in the audit function, and maybe prefix image by controlPlane

@axel7083
Copy link
Contributor Author

I was hesitant to do that instead, but maybe some people would want to use custom image, and making a dropdown force use to ask which version do we support ? How far do we go under the minor version ? Some version go from X.X.0 to X.X.11, that would be very long dropdown. If we select only major version, people could desire to have more control over the version used.

Yes there is simplicity over configurability here.

maybe 2 fields like one for the lazy people and another for the 'custom' value

one dropdown with latest, then some recent major versions like 1.27, 1.26 and then you have custom where it'll use the field that you're using

it's not blocking the PR, just that I think from usability, just selecting 1.26 or 1.27 is easier than going to an external site, looking for a string, copy pasting and coming back to enter the value.

I was not able to find a way to disable a field based on item value.
Based on your suggestion, a dropdown with major version, and a latest and custom. When custom is selected, having a text field shown or enable to let the user write in it.

How to made such mechanism with the existing system ?

The configuration does not really allows some kind of conditions

export interface IConfigurationPropertySchema {
id?: string;
type?: IConfigurationPropertySchemaType | IConfigurationPropertySchemaType[];
// eslint-disable-next-line @typescript-eslint/no-explicit-any
default?: any;
description?: string;
placeholder?: string;
markdownDescription?: string;
minimum?: number;
maximum?: number | string;
format?: string;
scope?: ConfigurationScope;
readonly?: boolean;
hidden?: boolean;
enum?: string[];
}

And I was not able to find a way to change dynamically those value. If you know any way of doing it I would appreciate !

@benoitf
Copy link
Collaborator

benoitf commented Aug 11, 2023

for now you won't have a dynamic field that will be displayed (it will be like 2 fields)

select your version

If other, please specify the image name

(you can report error in the audit part like if I select latest but add an image in the other field)

this PR https://github.com/containers/podman-desktop/pull/3251/files will bring a when clause so it's nice to be able to show/hide fields

Maybe with that, we could have in the audit field, a set of a property that then display the field

you can also focus on the custom image in this PR and in a later PR (or not) have dropdown / when clause, etc.

@axel7083
Copy link
Contributor Author

axel7083 commented Aug 11, 2023

for now you won't have a dynamic field that will be displayed (it will be like 2 fields)

select your version

If other, please specify the image name

(you can report error in the audit part like if I select latest but add an image in the other field)

this PR https://github.com/containers/podman-desktop/pull/3251/files will bring a when clause so it's nice to be able to show/hide fields

Maybe with that, we could have in the audit field, a set of a property that then display the field

you can also focus on the custom image in this PR and in a later PR (or not) have dropdown / when clause, etc.

Okey I would rather for this PR to focus on the custom image and having a dropdown with some major version later when the PR about the when condition will be merged.

I resolved the comments with your suggestions

@benoitf benoitf dismissed their stale review August 11, 2023 16:25

code has been updated 🚀

@benoitf
Copy link
Collaborator

benoitf commented Aug 11, 2023

thanks @axel7083 it looks nice, I will do additional tests but I won't probably merge new stuff a late Friday :-)

@axel7083
Copy link
Contributor Author

thanks @axel7083 it looks nice, I will do additional tests but I won't probably merge new stuff a late Friday :-)

Yeah no worries

@benoitf
Copy link
Collaborator

benoitf commented Aug 11, 2023

@axel7083 so it looks you'll need to tweak the code to match the styles (you can check that using yarn format:check and yarn lint:check (the PR check is failing)

these checks are also performed automatically when committing so you should have somehow them disabled :-)

@benoitf
Copy link
Collaborator

benoitf commented Aug 11, 2023

@axel7083 it's missing 'fotmat:fix'

do you have husky setting up properly the got hooks ? as you shouldn't be able to push commits without proper formatting

Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on the audit check, it needs to be cumulative and not first is the winner

@axel7083
Copy link
Contributor Author

@benoitf The pr-check / smoke e2e tests (pull_request) failed, any idea why ?

…g kind cluster

Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
axel7083 and others added 6 commits August 14, 2023 15:37
Co-authored-by: Florent BENOIT <fbenoit@redhat.com>
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
Co-authored-by: Florent BENOIT <fbenoit@redhat.com>
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com>
@benoitf benoitf force-pushed the feature/kind-custom-image-support branch from 7f7a0e9 to 6eecbb4 Compare August 14, 2023 13:37
@benoitf
Copy link
Collaborator

benoitf commented Aug 14, 2023

@axel7083 a flaky test

@benoitf benoitf merged commit 475c31c into containers:main Aug 14, 2023
8 checks passed
@benoitf
Copy link
Collaborator

benoitf commented Aug 14, 2023

thanks @axel7083 for your PR

@axel7083
Copy link
Contributor Author

thanks @axel7083 for your PR

Thanks you for all of your comments and time ! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants