-
Notifications
You must be signed in to change notification settings - Fork 93
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
Get kubernetes version for all cloud providers + pytest refactor #927
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.
LGTM, but as I was already tired in the end, it should be interesting to have someone else inspect the pytest parts XD. But in general, that's an amazing job, thanks @iameskild !!!!
config["profiles"] = DEFAULT_PROFILES | ||
config["environments"] = DEFAULT_ENVIRONMENTS | ||
config["profiles"] = DEFAULT_PROFILES.copy() | ||
config["environments"] = DEFAULT_ENVIRONMENTS.copy() | ||
|
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.
Is it possible to add the Kubernetes version checker here?
### Verify Kubernetes version
_set_kubernetes_version(config, kubernetes_version, cloud_provider)
Then we could have just one call for it, and remove it from the conditionals above
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.
@viniciusdc the current state requires that cloud_provider
be passed to the _set_kubernetes_version
which means we can't simply replace all of these functions calls with one call. We might be able to do something like:
for cloud_provider in ["aws", "do", "azure", "gcp", "local"]:
_set_kubernetes_version(config, kubernetes, cloud_provider)
preserved_filename = preserved_directory / "file.txt" | ||
preserved_filename.write_text("This is a test...") | ||
|
||
yield (qhub_config_loc, render_config_inputs) |
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.
LGTM, but it would be good to have someone else to have a look as well
Co-authored-by: Vinicius D. Cerutti <[email protected]>
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.
Excellent, thank you!
Fixes | Closes | Resolves #854 #628
Changes:
Besides adding/updating
kubernetes_versions
functions for all the cloud providers, I have made the following motivated changes:kubernetes_versions
functions behave. For all cloud providers, they return a list of available kubernetes versions from oldest to latest.QHUB_K8S_VERSION
environment variable.qhub init
, the latest available version will be used.ValueError
render_config
(qhub init
andqhub render
) will check the kubernetes version using the cloud provider SDK. This will require setting the appropriate credentials.setup_fixture
which can be reused for any test that callsrender_config
, reducing the amount of duplicated code.Types of changes
What types of changes does your code introduce?
Put an
x
in the boxes that applyTesting
Requires testing
In case you checked yes, did you write tests?
Large test suit refactor.
Further comments (optional)
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered and more.