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.

Sign up for GitHub

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

Jump to bottom

Add kubectl config flag to disable validation #3512

Merged
merged 1 commit into from
Jan 22, 2020
Merged

Add kubectl config flag to disable validation #3512

merged 1 commit into from
Jan 22, 2020

Conversation

zmb3
Copy link
Contributor

@zmb3 zmb3 commented Jan 16, 2020

Fixes #3222

Relates to n/a

Should merge before : n/a

Should merge after : n/a

Description

Add config flag to disable validation when using the kubectl deployer.
When enabled, --validate=false is passed to the kubectl apply and kubectl create commands.

User facing changes

n/a

Before

n/a

After

n/a

Next PRs.

n/a

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes unit tests
  • Mentions any output changes.
  • Adds documentation as needed: user docs, YAML reference, CLI reference.
  • Adds integration tests if needed.

Reviewer Notes

  • The code flow looks good.
  • Unit test added.
  • User facing changes look good.

Release Notes

  • Add config option to disable validation when using the kubectl deployer.

@dgageot dgageot self-assigned this Jan 16, 2020
@dgageot dgageot added the kokoro:run runs the kokoro jobs on a PR label Jan 16, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jan 16, 2020
dgageot
dgageot suggested changes Jan 16, 2020
View reviewed changes
pkg/skaffold/schema/latest/config.go Outdated
@@ -392,6 +392,10 @@ type KubectlDeploy struct {
// RemoteManifests lists Kubernetes manifests in remote clusters.
RemoteManifests []string `yaml:"remoteManifests,omitempty"`

// DisableValidation passes the `--valdiate=false` flag to supported
Copy link
Contributor

Choose a reason for hiding this comment

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

validate

pkg/skaffold/schema/latest/config.go Outdated
// DisableValidation passes the `--valdiate=false` flag to supported
// `kubectl` commands when enabled.
DisableValidation bool `yaml:"validate,omitempty"`

// Flags are additional flags passed to `kubectl`.
Flags KubectlFlags `yaml:"flags,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if DisableValidation could be under KubectlFlags somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we want it under KubectlFlags, because those flags are applied for any kubectl command we run, and --validate is only a valid option on some commands.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant, could we add a DisableValidation field to KubectlFlags and apply that to supported kubectl commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, right. That makes a lot of sense!

@codecov
Copy link

codecov bot commented Jan 16, 2020 •
edited
Loading

Codecov Report

Merging #3512 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted Files Coverage Δ
pkg/skaffold/schema/latest/config.go 100% <ø> (ø) ⬆️
pkg/skaffold/deploy/kubectl/cli.go 94.73% <100%> (+0.61%) ⬆️

@dgageot dgageot added pr/changes-requested kokoro:run runs the kokoro jobs on a PR labels Jan 16, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jan 16, 2020
dgageot
dgageot suggested changes Jan 16, 2020
View reviewed changes
pkg/skaffold/deploy/kubectl.go Outdated
CLI: kubectl.NewFromRunContext(runCtx),
Flags: runCtx.Cfg.Deploy.KubectlDeploy.Flags,
ForceDeploy: runCtx.Opts.Force,
DisableValidation: runCtx.Cfg.Deploy.KubectlDeploy.Flags.DisableValidation,
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is not required, now

pkg/skaffold/deploy/kubectl/cli.go Outdated
ForceDeploy bool
previousApply ManifestList
ForceDeploy bool
DisableValidation bool
Copy link
Contributor

Choose a reason for hiding this comment

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

This is included in Flags

pkg/skaffold/deploy/kubectl/cli.go Outdated
@@ -62,6 +63,10 @@ func (c *CLI) Apply(ctx context.Context, out io.Writer, manifests ManifestList)
args = append(args, "--force", "--grace-period=0")
}

if c.DisableValidation {
Copy link
Contributor

Choose a reason for hiding this comment

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

c. Flags. DisableValidation

pkg/skaffold/deploy/kubectl/cli.go Outdated
@@ -77,6 +82,10 @@ func (c *CLI) ReadManifests(ctx context.Context, manifests []string) (ManifestLi
}

args := c.args([]string{"--dry-run", "-oyaml"}, list...)
if c.DisableValidation {
Copy link
Contributor

Choose a reason for hiding this comment

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

c. Flags. DisableValidation

dgageot
dgageot suggested changes Jan 17, 2020
View reviewed changes
pkg/skaffold/schema/latest/config.go Outdated

// DisableValidation passes the `--validate=false` flag to supported
// `kubectl` commands when enabled.
DisableValidation bool `yaml:"validate,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for making all the changes!
One more problem and we should be good to go:
DisableValidation can't be called validate in the yaml. The first one is a negation. disableValidation seems an ok name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review and patience as I try to squeeze this in my mornings :-)

@dgageot
Copy link
Contributor

dgageot commented Jan 17, 2020

Oh, I'm sorry @zmb3, we released the v2alpha2 schema yesterday. I'll have to prepare a new v2alpha3 schema that you can rebase on top of. I won't do it before Monday, though.

@dgageot
Copy link
Contributor

dgageot commented Jan 17, 2020

@zmb3 Should be ready for a rebase

@zmb3
Copy link
Contributor Author

zmb3 commented Jan 17, 2020

Thanks @dgageot, that was fast for not before Monday!

@dgageot dgageot added the kokoro:run runs the kokoro jobs on a PR label Jan 22, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jan 22, 2020
@dgageot
Copy link
Contributor

dgageot commented Jan 22, 2020

\o/

@dgageot dgageot merged commit 7d7f3c1 into GoogleContainerTools:master Jan 22, 2020
@zmb3 zmb3 deleted the zb-add-kubectl-validate-flag branch January 27, 2020 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgageot dgageot dgageot approved these changes

@balopat balopat Awaiting requested review from balopat

@nkubala nkubala Awaiting requested review from nkubala

@tejal29 tejal29 Awaiting requested review from tejal29

Assignees

@dgageot dgageot

Labels
cla: yes pr/changes-requested size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kubectl deployer doesn't offer a way to skip validation
4 participants
@zmb3 @dgageot @googlebot @kokoro-team