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

feat: add k8s manifest support to skaffold lint and one sample rule #6795

Conversation

aaron-prindle
Copy link
Contributor

@aaron-prindle aaron-prindle commented Nov 3, 2021

Related to #6098, adds k8s manifest support to skaffold lint and one sample rule:

  • K8sManifestManagedByLabelInUse

The sample rule has the following output when triggered (note skaffold lint is run against modified version of examples/microservices here w/ misconfigurations added to the Dockerfiles, not the actual examples/microservices):

$ skaffold lint
leeroy-web/kubernetes/deployment.yaml:7:35: ID000004: Found usage of label 'app.kubernetes.io/managed-by'.  skaffold overwrites the 'app.kubernetes.io/managed-by' field to 'app.kubernetes.io/managed-by: skaffold'. and as such is recommended to remove this label: (YamlFieldLintRule)
    app.kubernetes.io/managed-by: helm
                                  ^

Future work for #6098 includes implementing the top 2 lint rules for k8s manifests on top of the framework in this PR.

@aaron-prindle aaron-prindle requested a review from a team as a code owner November 3, 2021 20:23
@aaron-prindle aaron-prindle requested a review from tejal29 November 3, 2021 20:23
@google-cla google-cla bot added the cla: yes label Nov 3, 2021
@aaron-prindle aaron-prindle force-pushed the fix-6098_k8s-manifests-support-v3 branch 2 times, most recently from 4bad89b to 428bff0 Compare November 3, 2021 21:03
@codecov
Copy link

codecov bot commented Nov 3, 2021

Codecov Report

Merging #6795 (1b388a7) into main (290280e) will decrease coverage by 1.19%.
The diff coverage is 62.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6795      +/-   ##
==========================================
- Coverage   70.48%   69.28%   -1.20%     
==========================================
  Files         515      544      +29     
  Lines       23150    24823    +1673     
==========================================
+ Hits        16317    17199     +882     
- Misses       5776     6472     +696     
- Partials     1057     1152      +95     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/deploy.go 52.00% <ø> (-1.85%) ⬇️
cmd/skaffold/app/cmd/dev.go 84.61% <0.00%> (ø)
cmd/skaffold/app/cmd/flags.go 89.00% <0.00%> (-1.82%) ⬇️
cmd/skaffold/app/cmd/render.go 36.66% <0.00%> (-4.72%) ⬇️
cmd/skaffold/skaffold.go 0.00% <ø> (ø)
cmd/skaffold/app/cmd/lint.go 42.85% <42.85%> (ø)
cmd/skaffold/app/cmd/find_configs.go 48.88% <50.00%> (+0.24%) ⬆️
cmd/skaffold/app/cmd/cmd.go 70.49% <75.00%> (-0.57%) ⬇️
cmd/skaffold/app/cmd/delete.go 69.23% <80.00%> (+13.67%) ⬆️
cmd/skaffold/app/cmd/debug.go 100.00% <100.00%> (ø)
... and 140 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe0b543...1b388a7. Read the comment docs.

@MarlonGamez MarlonGamez self-assigned this Nov 4, 2021
@aaron-prindle aaron-prindle force-pushed the fix-6098_k8s-manifests-support-v3 branch from 428bff0 to f748cee Compare November 9, 2021 18:21
Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

minor nit

for _, pattern := range c.Deploy.KubectlDeploy.Manifests {
// NOTE: pattern is a pattern that can have wildcards, eg: leeroy-app/kubernetes/*
if util.IsURL(pattern) {
log.Entry(ctx).Infof("skaffold lint found url manifest and is skipping lint rules for: %s", pattern)
Copy link
Contributor

Choose a reason for hiding this comment

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

shd we move this to debug instead? Additional log lines to users for default level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, done

@aaron-prindle aaron-prindle force-pushed the fix-6098_k8s-manifests-support-v3 branch from f748cee to 1b388a7 Compare November 9, 2021 18:23
@tejal29 tejal29 enabled auto-merge (squash) November 9, 2021 18:48
@tejal29 tejal29 merged commit 096930f into GoogleContainerTools:main Nov 9, 2021
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.

3 participants