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

Use TaskConfigValidator for RunnerTask #554

Merged
merged 4 commits into from
Sep 18, 2019

Conversation

dirceu
Copy link
Contributor

@dirceu dirceu commented Sep 12, 2019

What are you trying to accomplish with this PR?

Fix part of #344 by using all supported TaskConfigValidator validations in kubernetes-deploy run (plus additional ones to validate args and template).

How is this accomplished?

In the first commit I enabled all validations from TaskConfigValidator and updated error messages in tests accordingly. I also chose to delete the unit tests we had for RunnerTask because:

  1. There were only two and they were related to validation.
  2. It would be too much effort to mock all kubectl and kubeclient calls for a low ROI.

In the second commit I created RunnerTaskConfig and RunnerTaskConfigValidator and moved the validation of args and template there to follow the same pattern for all validations.

What could go wrong?

The same as #533: 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.

@dirceu dirceu force-pushed the inflight-validation-runner-task branch from 7969a80 to 85939db Compare September 13, 2019 14:18
@dirceu dirceu marked this pull request as ready for review September 13, 2019 15:14
@dirceu dirceu self-assigned this Sep 13, 2019
@dirceu dirceu requested a review from a team September 13, 2019 17:17
Copy link
Contributor

@dturn dturn left a comment

Choose a reason for hiding this comment

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

My two big concerns are if we really need a RunnerTaskConfig class. But also that we haven't lost coverage by the removed tests.

lib/kubernetes-deploy/common.rb Outdated Show resolved Hide resolved
lib/kubernetes-deploy/runner_task.rb Outdated Show resolved Hide resolved
lib/kubernetes-deploy/runner_task_config_validator.rb Outdated Show resolved Hide resolved
@dirceu dirceu force-pushed the inflight-validation-runner-task branch from 85939db to 71f7ce7 Compare September 16, 2019 13:06
@dirceu
Copy link
Contributor Author

dirceu commented Sep 16, 2019

also that we haven't lost coverage by the removed tests.

👍

I now added integration tests that make essentially the same assertions we had before, just in case.

@dirceu dirceu requested a review from dturn September 16, 2019 14:19
Copy link
Contributor

@dturn dturn left a comment

Choose a reason for hiding this comment

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

Changes look good, one quick question.

lib/kubernetes-deploy/runner_task_config_validator.rb Outdated Show resolved Hide resolved
@dirceu dirceu requested a review from a team September 16, 2019 18:21
Copy link
Contributor

@lei-lo lei-lo left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks for adding the logger statements :)

lib/kubernetes-deploy/runner_task.rb Outdated Show resolved Hide resolved
@dirceu dirceu force-pushed the inflight-validation-runner-task branch from 5a9ff64 to 34b3871 Compare September 18, 2019 12:05
@dirceu
Copy link
Contributor Author

dirceu commented Sep 18, 2019

Fixed rubocop warning and added changelog entry. Will merge once CI is green.

@dirceu dirceu merged commit 43dd8ae into master Sep 18, 2019
@dirceu dirceu deleted the inflight-validation-runner-task branch September 18, 2019 14:05
@douglas douglas mentioned this pull request Sep 20, 2019
@douglas douglas temporarily deployed to rubygems September 20, 2019 20:14 Inactive
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

5 participants