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

Task config kubectl #584

Merged
merged 1 commit into from
Oct 10, 2019
Merged

Task config kubectl #584

merged 1 commit into from
Oct 10, 2019

Conversation

dturn
Copy link
Contributor

@dturn dturn commented Oct 8, 2019

What are you trying to accomplish with this PR?

Make it easy to have a global-deploy command. #574 has shown that some objects validate too much, e.g. Kubectl validates its passes a namespace, but it shouldn't if it's only going to be used for global commands.

By pushing TaskConfig in more places it should become easier to feel good about limiting validations in non-task classes.

This doesn't finish #572, we still need to pass it to the Resource objects. But this already hits 19 files.

How is this accomplished?
Pass TaskConfig instances around. Remove some validations in Kubectl.

What could go wrong?
Test coverage is lacking ...

@dturn dturn force-pushed the task-config-kubectl branch 3 times, most recently from 23729be to 2d1b3c5 Compare October 8, 2019 18:24
@dturn dturn marked this pull request as ready for review October 8, 2019 18:32
@dturn dturn requested a review from a team October 8, 2019 18:33
@RyanBrushett RyanBrushett requested a review from a team October 9, 2019 15:42
Copy link
Contributor

@RyanBrushett RyanBrushett left a comment

Choose a reason for hiding this comment

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

Test coverage is lacking ...

Codecov seems happy enough. Anything you could do to make yourself more comfortable here?
Did you run any of the commands locally to see how it worked out?

@dturn
Copy link
Contributor Author

dturn commented Oct 10, 2019

Codecov seems happy enough. Anything you could do to make yourself more comfortable here?
Did you run any of the commands locally to see how it worked out?

Taking a look at codecov it does look like everything is covered. I'm not really worried about the tasks breaking.

Copy link
Contributor

@RyanBrushett RyanBrushett left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@dturn dturn merged commit f79659d into master Oct 10, 2019
@dturn dturn deleted the task-config-kubectl branch October 10, 2019 18:23
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.

3 participants