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

cli: store kubernetes version as strong type in config #2287

Merged
merged 14 commits into from
Sep 19, 2023

Conversation

elchead
Copy link
Contributor

@elchead elchead commented Aug 26, 2023

Context

Storing the K8s version as validated type saves a lot of parameter passing and centralizes the validation logic

Proposed change(s)

  • store K8s version as weakly validated type (ignoring patch version) in the config
  • allow to specify K8s version without patch (e.g. 1.27) as valid config value and resolve the patch version
  • allow to specify a patch version in config generate (e.g. 1.27.4)
  • add more unit test cases

Related issue

Checklist

  • Update docs
  • Add labels (e.g., for changelog category)
  • Is PR title adequate for changelog?
  • Link to Milestone

@netlify
Copy link

netlify bot commented Aug 26, 2023

Deploy Preview for constellation-docs canceled.

Name Link
🔨 Latest commit bff1570
🔍 Latest deploy log https://app.netlify.com/sites/constellation-docs/deploys/650971797e787a0008d91112

@elchead elchead force-pushed the ref/config/k8s-version-validation branch 2 times, most recently from 98a8f44 to eb60156 Compare August 27, 2023 12:25
@elchead elchead added the no changelog Change won't be listed in release changelog label Aug 27, 2023
@elchead elchead force-pushed the ref/config/k8s-version-validation branch from 8d4087f to 3f824b4 Compare August 28, 2023 09:27
@elchead elchead marked this pull request as ready for review August 28, 2023 09:29
@elchead elchead requested a review from 3u13r as a code owner August 28, 2023 09:29
@elchead elchead added this to the v2.11.0 milestone Aug 28, 2023
@elchead
Copy link
Contributor Author

elchead commented Aug 28, 2023

Discuss: supporting versions without patch as K8s version might be worth putting in the release notes

cli/internal/cmd/init.go Outdated Show resolved Hide resolved
cli/internal/cmd/init.go Outdated Show resolved Hide resolved
@elchead elchead requested a review from derpsteb August 28, 2023 11:18
@elchead
Copy link
Contributor Author

elchead commented Aug 28, 2023

internal/config/config_test.go Outdated Show resolved Hide resolved
internal/versions/versions.go Outdated Show resolved Hide resolved
internal/versions/versions.go Outdated Show resolved Hide resolved
internal/versions/versions.go Outdated Show resolved Hide resolved
@elchead
Copy link
Contributor Author

elchead commented Aug 28, 2023

@derpsteb I reverted the support for no patch specification in the config due to complicated case handling. I think it's not worth to support it, but have saved the stash if we decide to do so in the future

@elchead elchead force-pushed the ref/config/k8s-version-validation branch from 04a0f56 to 9c54bc0 Compare September 5, 2023 07:08
@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2023

Coverage report

Package Old New Trend
cli/internal/cmd 56.30% 55.00% ↘️
cli/internal/helm 49.40% 49.50% ↗️
cli/internal/kubecmd 58.00% 58.00% ↔️
e2e/internal/upgrade [no test files] [no test files] 🚧
internal/config 79.80% 80.10% ↗️
internal/config/migration [no test files] [no test files] 🚧
internal/versions 13.90% 8.20% ↘️

@elchead
Copy link
Contributor Author

elchead commented Sep 5, 2023

Coverage report

Package Old New Trend
cli/internal/cmd 53.60% 53.40% ↘️
cli/internal/helm 47.70% 47.80% ↗️
cli/internal/kubecmd 57.70% 57.30% ↘️
e2e/internal/upgrade [no test files] [no test files] 🚧
internal/config 79.70% 80.00% ↗️
internal/config/migration [no test files] [no test files] 🚧
internal/versions 13.90% 8.10% ↘️

What's interesting about these metrics is that it doesn't encourage to test closer to E2E. I.e, the added test cases for the valid Kubernetes flag values in config generate do not add to the test coverage because it exercises code from the versions pkg. Would it be useful to also have a global coverage metric?

@elchead elchead force-pushed the ref/config/k8s-version-validation branch from 9a9916e to 9462b70 Compare September 8, 2023 09:18
@elchead elchead force-pushed the ref/config/k8s-version-validation branch from 9462b70 to 7d3ce1f Compare September 8, 2023 14:51
@elchead elchead force-pushed the ref/config/k8s-version-validation branch from 7d3ce1f to 038d9f8 Compare September 11, 2023 07:58
@3u13r 3u13r modified the milestones: v2.11.0, v2.12.0 Sep 11, 2023
3u13r
3u13r previously requested changes Sep 11, 2023
cli/internal/cmd/init.go Outdated Show resolved Hide resolved
@elchead elchead force-pushed the ref/config/k8s-version-validation branch from 038d9f8 to 36a7e74 Compare September 11, 2023 11:19
@elchead elchead requested a review from 3u13r September 11, 2023 11:19
@elchead
Copy link
Contributor Author

elchead commented Sep 18, 2023

@3u13r can u give it another look pls when u find a moment?

Copy link
Member

@3u13r 3u13r left a comment

Choose a reason for hiding this comment

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

One small nit, otherwise LGTM

cli/internal/cmd/configgenerate.go Outdated Show resolved Hide resolved
@elchead elchead force-pushed the ref/config/k8s-version-validation branch from 36a7e74 to bff1570 Compare September 19, 2023 10:01
@elchead elchead merged commit 22c2a73 into main Sep 19, 2023
@elchead elchead deleted the ref/config/k8s-version-validation branch September 19, 2023 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Change won't be listed in release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants