✨Fetch latest provider version if it's not set in the spec #180

Fedosin · 2023-06-25T16:59:03Z

What this PR does / why we need it:

Now users have to manually set provider version, which is not convenient as it requires additional github searching. This PR allows to omit it, and the latest version will be substituted automatically. This behavior is similar to what is done in clusterctl, where version is optional.

Additionally this PR adds e2e tests for air-gapped environments.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

api/v1alpha1/provider_types.go

alexander-demicev · 2023-06-26T11:19:21Z

internal/controller/phases.go

+	var err error
+
+	// Create repository either from provided URL or from a config map.
+	if p.provider.GetSpec().FetchConfig != nil && p.provider.GetSpec().FetchConfig.Selector != nil {


can we optimize it and initialize the repo only once? It gets created in the later phases too

yeah, I slightly rewrote the original PR, so it is more optimized now.

Fedosin · 2023-06-27T16:16:39Z

/hold

@alexander-demicev agree, we can optimise this code a little. I'll propose another update for this PR soon

Fedosin · 2023-07-01T10:07:48Z

/hold cancel

Fedosin · 2023-07-01T16:43:52Z

/retest

Fedosin · 2023-07-01T18:23:28Z

/retest

Fedosin · 2023-07-01T18:48:11Z

/retest

alexander-demicev · 2023-07-04T13:18:16Z

internal/controller/manifests_downloader.go

+		spec.Version = repo.DefaultVersion()
+
+		// Add version to the provider spec.
+		p.provider.SetSpec(spec)


why provider spec is set twice?

The reason spec is set twice is because it accommodates two distinct workflows:

When downloading manifests from GitHub/GitLab.

When working with config maps containing manifests.

For the first workflow, the version is set during the manifest download process when a fetch URL is used. In the case of config maps, the version setting occurs in the load phase. Regardless of the workflow, the spec update is executed only once.

furkatgofurov7

What happens in case, CAPI tags v1.5.x as latest while CAPZ did not yet uplift to latest of CAPI and has latest tagged version compatible only with CAPI v1.4.x?

furkatgofurov7 · 2023-07-04T14:09:39Z

test/e2e/air_gapped_test.go

+
+		configMaps := []corev1.ConfigMap{}
+
+		for _, fileName := range []string{"core-cluster-api-v1.4.2.yaml", "core-cluster-api-v1.4.3.yaml"} {


perhaps this the only way to do it in air gapped env (manually updating/adding huge manifests as CAPI releasing new versions), but I do not have a better suggestion either how we could improve it..

yes, this is the only way. For clusterctl you have to create local repositories, for operator you can download manifests from github and make a configmap from file.

alexander-demicev · 2023-07-04T14:31:09Z

@furkatgofurov7 I guess the issue here is similar to when using clusterctl, you have to specify version in case you want to be sure that dependencies are aligned

furkatgofurov7 · 2023-07-04T14:38:52Z

@furkatgofurov7 I guess the issue here is similar to when using clusterctl, you have to specify version in case you want to be sure that dependencies are aligned

That is true and I guess it is fine to follow the same behaviour here

alexander-demicev

/approve

k8s-ci-robot · 2023-07-07T11:50:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexander-demicev

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~~ [alexander-demicev]

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

furkatgofurov7

/lgtm

k8s-ci-robot · 2023-07-07T13:12:12Z

LGTM label has been added.

Git tree hash: 63a6154b1aab26d415d72dfa1308d997718ed432

furkatgofurov7 · 2023-07-07T13:12:32Z

/hold
to address #180 (comment)

Fedosin · 2023-07-09T20:21:42Z

/hold cancel

k8s-ci-robot requested a review from alexander-demicev June 25, 2023 16:59

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 25, 2023

k8s-ci-robot requested a review from JoelSpeed June 25, 2023 16:59

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 25, 2023

alexander-demicev reviewed Jun 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 Jun 27, 2023

api: make "version" in the provider spec optional

e97b33b

Fedosin force-pushed the latest_version branch from c4a9064 to 978685b Compare July 1, 2023 10:06

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

Fedosin force-pushed the latest_version branch 3 times, most recently from 2faa5a1 to d81e26f Compare July 1, 2023 16:01

Fedosin force-pushed the latest_version branch 3 times, most recently from 04a3cf9 to 57264d7 Compare July 1, 2023 18:13

feat: set latest provider version if spec doesn't define it

9f9257f

Fedosin force-pushed the latest_version branch from 57264d7 to 5914e4b Compare July 1, 2023 18:31

test: add tests for the version setter

9e4a035

Fedosin force-pushed the latest_version branch from 5914e4b to 9e4a035 Compare July 1, 2023 19:03

alexander-demicev reviewed Jul 4, 2023

View reviewed changes

furkatgofurov7 reviewed Jul 4, 2023

View reviewed changes

alexander-demicev approved these changes Jul 7, 2023

View reviewed changes

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

furkatgofurov7 reviewed Jul 7, 2023

View reviewed changes

k8s-ci-robot assigned furkatgofurov7 Jul 7, 2023

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

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

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

k8s-ci-robot merged commit e1a5c7b into kubernetes-sigs:main Jul 9, 2023


		configMaps := []corev1.ConfigMap{}

		for _, fileName := range []string{"core-cluster-api-v1.4.2.yaml", "core-cluster-api-v1.4.3.yaml"} {

✨Fetch latest provider version if it's not set in the spec #180

✨Fetch latest provider version if it's not set in the spec #180

Uh oh!

Conversation

Fedosin commented Jun 25, 2023

Uh oh!

Uh oh!

alexander-demicev Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

Fedosin Jul 1, 2023

Choose a reason for hiding this comment

Uh oh!

Fedosin commented Jun 27, 2023

Uh oh!

Fedosin commented Jul 1, 2023

Uh oh!

Fedosin commented Jul 1, 2023

Uh oh!

Fedosin commented Jul 1, 2023

Uh oh!

Fedosin commented Jul 1, 2023

Uh oh!

alexander-demicev Jul 4, 2023

Choose a reason for hiding this comment

Uh oh!

Fedosin Jul 9, 2023

Choose a reason for hiding this comment

Uh oh!

furkatgofurov7 left a comment

Choose a reason for hiding this comment

Uh oh!

furkatgofurov7 Jul 4, 2023

Choose a reason for hiding this comment

Uh oh!

alexander-demicev Jul 4, 2023

Choose a reason for hiding this comment

Uh oh!

alexander-demicev commented Jul 4, 2023

Uh oh!

furkatgofurov7 commented Jul 4, 2023

Uh oh!

alexander-demicev left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jul 7, 2023

Uh oh!

furkatgofurov7 left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jul 7, 2023

Uh oh!

furkatgofurov7 commented Jul 7, 2023

Uh oh!

Fedosin commented Jul 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants