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

First pass at shared validation #533

Merged
merged 7 commits into from
Aug 30, 2019
Merged

First pass at shared validation #533

merged 7 commits into from
Aug 30, 2019

Conversation

dturn
Copy link
Contributor

@dturn dturn commented Aug 16, 2019

What are you trying to accomplish with this PR?
Consistent preflight validations for all tasks. Inspiration for this PR taken from https://github.com/Shopify/kubernetes-deploy/compare/config_validator

I've started by just adding this to restart task. If we like where this is going I can add it to the remaining tasks in this PR or subsequent ones.

How is this accomplished?

Our run/restart/deploy tasks have overlapping state they need, I've attempted to extract it into TaskConfig object. For now this is context, namespace, and logger. I've also added methods for generating kubectl, and kubeclient_builder, its possible that this is backwards and those methods should take a task_config. (We decided against generating kubectl, kubeclient_builder in task_config, see the comments for the discussion about why). When we go to Krane I think the TaskConfig should be passed to the task instead of it building it. (Though maybe not since that would require users using this as a library to construct a 'random' object, perhaps two constructors for each task).

I've also created a Validator object that can validate, kubeconfig, context, namespace and kubectl version. This looks to be the shared set of validations. Each task has additional validations, this PR leaves open several approaches to solving that problem.

What could go wrong?

Because we need to unify validations that are done slightly differently we are changing the error class that gets raised as well as the wording. If people were rescuing these errors or matching text this will be a breaking change.

lib/kubernetes-deploy/task_config.rb Outdated Show resolved Hide resolved
lib/kubernetes-deploy/restart_task.rb Outdated Show resolved Hide resolved
lib/kubernetes-deploy/validator.rb Outdated Show resolved Hide resolved
@dturn dturn self-assigned this Aug 23, 2019
@dturn dturn force-pushed the consistent-validations branch 2 times, most recently from ce62469 to 6edce21 Compare August 26, 2019 22:36
Copy link
Contributor

@timothysmith0609 timothysmith0609 left a comment

Choose a reason for hiding this comment

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

A few small comments, but this looks ready to go once you address them.
Have you identified other parts of the code where we can start substituting in @task_config, or do you feel it's only really going to be worthwhile for the purposes of validation?

test/integration/task_config_validator_test.rb Outdated Show resolved Hide resolved
test/unit/task_config_test.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@timothysmith0609 timothysmith0609 left a comment

Choose a reason for hiding this comment

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

Noticed a few typographical things on this pass, but everything else looks good for me

test/integration/task_config_validator_test.rb Outdated Show resolved Hide resolved
test/integration/task_config_validator_test.rb Outdated Show resolved Hide resolved
test/integration/task_config_validator_test.rb Outdated Show resolved Hide resolved
test/unit/task_config_test.rb Outdated Show resolved Hide resolved
test/integration/restart_task_test.rb Outdated Show resolved Hide resolved
lib/kubernetes-deploy/task_config_validator.rb Outdated Show resolved Hide resolved
lib/kubernetes-deploy/task_config_validator.rb Outdated Show resolved Hide resolved
@dturn dturn requested a review from a team August 29, 2019 21:05
@dturn dturn merged commit 29e29a5 into master Aug 30, 2019
@dturn dturn deleted the consistent-validations branch August 30, 2019 17:50
@timothysmith0609 timothysmith0609 mentioned this pull request Sep 4, 2019
@timothysmith0609 timothysmith0609 temporarily deployed to rubygems September 5, 2019 12:08 Inactive
@ghost ghost added the krane [ProdX-GSD] label Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants