feat(linting): Add support for linting decK files #981

mheap · 2023-07-24T09:21:42Z

Adds support for linting declarative configurations.

Example usage:

deck file lint -s ./kong.yaml -r ruleset.yaml

You can control what's considered an error:

deck file lint -s ./kong.yaml -r ruleset.yaml --fail-severity info

Or only show errors:

deck file lint -s ./kong.yaml -r ruleset.yaml -D

It supports JSON and YAML output in addition to a human readable default:

Linting Violations: 2
Failures: 1

[9:15] Must use HTTPS protocol: `http` does not match the expression `^https`
[1:18] Check the version is correct: `3.0` does not match the expression `^1.1$`
exit status 1

Tested with the following input files:

# kong.yaml
_format_version: "3.0"
services:
  - host: mockbin.org
    id: b1525aee-d304-11ed-afa1-0242ac120002
    name: summer-time
    path: /requests
    plugins: []
    port: 443
    protocol: http
    routes:
      - id: b7d87736-d304-11ed-afa1-0242ac120002
        methods:
          - GET
        name: summer-time_get
        paths:
          - ~/summer-time$
        plugins: []
        regex_priority: 200
        strip_path: false
        tags: []

# ruleset.yaml
rules:
  version-30:
    description: "Check the version is correct"
    given: $._format_version
    severity: error
    then:
      function: pattern
      functionOptions:
        match: "^1.1$"
  https-only:
    description: "Must use HTTPS protocol"
    given: $.services[*].protocol
    severity: warn
    then:
      function: pattern
      functionOptions:
        match: "^https"

Usage:

Usage:
  deck file lint [flags] lint-file

Flags:
  -D, --display-only-failures   only display failures
      --fail-severity string    minimum severity to fail on (default "warn")
      --format string           output format: default, json or yaml (default "default")
  -h, --help                    help for lint
  -r, --ruleset string          Ruleset to apply to the state file.
  -s, --state string            decK file to process. Use - to read from stdin. (default "-")

Jira: APIOPS-53

edit: added jira link

CLAassistant · 2023-07-24T09:21:50Z

All committers have signed the CLA.

GGabriele

Nice work!

I added a few suggestions and requests in the code. On top of those, can you please make sure to rebase on top of main and also add some basic test? Something like this should be relatively easy to replicate.

cmd/file_lint.go

GGabriele · 2023-08-22T18:45:35Z

cmd/file_lint.go

+)
+
+var (
+	cmdLintInputFilename string


These 2 variables are never used.

Also, not passing a statefile and using - as default doesn't work:

$ ./deck file lint - --ruleset ruleset.yaml panic: open -: no such file or directory

It's not clear what lint-file is in the usage, so this usage appears undefined to me. If we want to allow positional arguments, it seems we need to decide if that is the input state file(s) or the rules file(s). Can the underlying library support multiple rules files? It may make sense to allow a single input state file via -s,--state and multiple rules files as the positional arguments.

Can the underlying library support multiple rules files?

The library gets a single input, so it's a matter of implementing the logic in decK to merge the rulesets.

It may make sense to just support a single state file and a single ruleset for starters. WDYT?

I don’t see a use case for supporting multiple at this point. Users can run the tool multiple times, or use the merge function to generate a single input

cmd/file_lint.go

GGabriele · 2023-08-22T21:42:39Z

cmd/file_lint.go

-		Long:  `Long description Here`,
-		RunE:  executeLint,
+		Short: "Lint a file against a ruleset",
+		Long: "Lint a file against a ruleset. \n" +


@mheap @rspurgeon can you please validate these?

I'm still not sure what lint-file represents in this usage. I think we're using flags for both the state file and the rules file, so what is lint-file?

As far as help text:

Suggested change

Long: "Lint a file against a ruleset. \n" +

Long: "Validate a decK state file against a linting ruleset, reporting any violations or failures. Report output can be returned in JSON, YAML, or human readable format (see --format). \n" +

I also think it's important we document the --fail-severity flag and how it affects the return code of the command. I assume that users of this will want to branch behaviors based on command return code. I think we should provide details here. I would make a suggestion, but I'm not sure at this point how the code works in that regard.

Thanks for the suggestion!

Fixed this. 3bdd2fe

I would make a suggestion, but I'm not sure at this point how the code works in that regard.

Basically --fail-severity will gate what returns a non-zero (1) exit code. Feel free to make a suggestion.

Also, is --display-only-failures expected to care about error only or it should adapt to the --fail-severity value? If the --display-only-failures flag should care only about error I suggest we change it to --display-only-errors.

Thoughts? @rspurgeon @mheap

--fail-severity and --display-only-failures are taken from Spectral, which is the defacto implementation of linting https://docs.stoplight.io/docs/spectral/9ffa04e052cc1-spectral-cli

We should adapt to the --fail-severity value

From Spectral:

-D, --display-only-failures only output results equal to or greater than --fail-severity [boolean] [default: false]

@mheap thanks for the clarifying!

a4e025c

Can we just adopt what the spectral CLI supports directly? Can we update our usage to be:

-F, --fail-severity results of this level or above will trigger a failure exit code [string] [choices: "error", "warn", "info", "hint"] [default: "error"] -D, --display-only-failures only output results equal to or greater than --fail-severity [boolean] [default: false]

codecov-commenter · 2023-08-22T22:07:19Z

Codecov Report

Attention: 122 lines in your changes are missing coverage. Please review.

Comparison is base (30f19db) 33.50% compared to head (70f4eb2) 33.18%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #981      +/-   ##
==========================================
- Coverage   33.50%   33.18%   -0.33%     
==========================================
  Files         101      102       +1     
  Lines       12460    12582     +122     
==========================================
  Hits         4175     4175              
- Misses       7878     8000     +122     
  Partials      407      407

Files	Coverage Δ
cmd/root.go	`0.88% <0.00%> (-0.01%)`	⬇️
cmd/file_lint.go	`0.00% <0.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rspurgeon · 2023-08-23T13:33:03Z

Should we output the severity in plaintext mode? It is provided in JSON or YAML format and I think is helpful for adjusting the rules or severity threshold.

./deck file lint -s ~/dev/tmp/kong-mock.yaml -r ~/dev/tmp/ruleset.yaml
Linting Violations: 2
Failures: 2

[10:15] Must use HTTPS protocol: `http` does not match the expression `^https`
[2:18] Check the version is correct: `3.0` does not match the expression `^1.1$`

Maybe something like:

[error][10:15] Must use HTTPS protocol: `http` does not match the expression `^https`
[warn][2:18] Check the version is correct: `3.0` does not match the expression `^1.1$`

GGabriele · 2023-08-23T13:55:50Z

@rspurgeon I think I addressed both your latest comments. Let me know!

rspurgeon

LGTM, impressive work @mheap and @GGabriele

cmd/file_lint.go

mikefero

LGTM. @mheap Do we plan on creating docs for this new feature and give some guidance to users (or even create a few real world examples)?

rspurgeon · 2023-09-12T19:05:53Z

As discussed with @Tieske , I'd prefer we hold this for a new release version instead of merging into a revision release. We want to bundle this with other APIops related features that are slated for completion the coming weeks. Part of that work includes public docs, blog announcement, etc...

Tieske · 2023-10-17T16:30:14Z

added a commit to line up the cli args with the other commands. Please review the last commit.

Tieske · 2023-10-17T16:31:39Z

As discussed with @Tieske , I'd prefer we hold this for a new release version instead of merging into a revision release. We want to bundle this with other APIops related features that are slated for completion the coming weeks. Part of that work includes public docs, blog announcement, etc...

@rspurgeon this no longer holds right? we can merge once approved?

rspurgeon · 2023-10-17T16:37:08Z

@rspurgeon this no longer holds right? we can merge once approved?

Yes, thank you... We are clear to ship incremental (non-breaking) features in minor releases. We should provide sufficient documentation both in product and on public docs site before merging, however.

mheap requested a review from a team July 24, 2023 09:21

mheap force-pushed the file-lint branch 2 times, most recently from 5823a9b to 8540606 Compare July 24, 2023 09:32

GGabriele requested changes Aug 22, 2023

View reviewed changes

GGabriele force-pushed the file-lint branch from 8540606 to b2c7ac1 Compare August 22, 2023 21:41

GGabriele requested a review from rspurgeon August 22, 2023 21:41

GGabriele reviewed Aug 22, 2023

View reviewed changes

GGabriele previously approved these changes Aug 23, 2023

View reviewed changes

GGabriele dismissed their stale review via 51e9292 August 23, 2023 13:48

rspurgeon approved these changes Aug 23, 2023

View reviewed changes

cmd/file_lint.go Outdated Show resolved Hide resolved

GGabriele force-pushed the file-lint branch from 49dab14 to 27ff489 Compare August 23, 2023 15:39

GGabriele requested a review from mikefero August 23, 2023 15:40

GGabriele requested a review from Tieske September 6, 2023 09:43

mikefero previously approved these changes Sep 6, 2023

View reviewed changes

Tieske dismissed mikefero’s stale review via 2795438 October 17, 2023 16:27

Tieske previously approved these changes Oct 17, 2023

View reviewed changes

Tieske dismissed their stale review via 53d35df October 18, 2023 11:14

mheap and others added 3 commits October 18, 2023 16:51

feat(linting): Add support for linting decK files

615dd07

chore(lint): slight refactor sync cli args with other cmds

5bbc770

chore(test): fix typo

70f4eb2

Tieske force-pushed the file-lint branch from 53d35df to 70f4eb2 Compare October 18, 2023 14:51

GGabriele approved these changes Oct 18, 2023

View reviewed changes

Tieske merged commit dc106ee into main Oct 18, 2023
35 checks passed

Tieske deleted the file-lint branch October 18, 2023 15:18

Tieske mentioned this pull request Nov 10, 2023

deck will not respond to <ctrl>+<c> when reading from stdin #1077

Closed

AntoineJac pushed a commit that referenced this pull request Jan 23, 2024

feat(linting): Add support for linting decK files (#981)

7cb2f98

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(linting): Add support for linting decK files #981

feat(linting): Add support for linting decK files #981

mheap commented Jul 24, 2023 •

edited by Tieske

Loading

CLAassistant commented Jul 24, 2023 •

edited

Loading

GGabriele left a comment

GGabriele Aug 22, 2023

rspurgeon Aug 22, 2023

GGabriele Aug 22, 2023

mheap Aug 22, 2023

GGabriele Aug 22, 2023

rspurgeon Aug 22, 2023

rspurgeon Aug 22, 2023

GGabriele Aug 22, 2023

GGabriele Aug 23, 2023 •

edited

Loading

mheap Aug 23, 2023

GGabriele Aug 23, 2023

rspurgeon Aug 23, 2023

codecov-commenter commented Aug 22, 2023 •

edited

Loading

rspurgeon commented Aug 23, 2023

GGabriele commented Aug 23, 2023

rspurgeon left a comment

mikefero left a comment

rspurgeon commented Sep 12, 2023

Tieske commented Oct 17, 2023

Tieske commented Oct 17, 2023

rspurgeon commented Oct 17, 2023

	Long: "Lint a file against a ruleset. \n" +
	Long: "Validate a decK state file against a linting ruleset, reporting any violations or failures. Report output can be returned in JSON, YAML, or human readable format (see --format). \n" +

feat(linting): Add support for linting decK files #981

feat(linting): Add support for linting decK files #981

Conversation

mheap commented Jul 24, 2023 • edited by Tieske Loading

CLAassistant commented Jul 24, 2023 • edited Loading

GGabriele left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GGabriele Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 22, 2023 • edited Loading

Codecov Report

rspurgeon commented Aug 23, 2023

GGabriele commented Aug 23, 2023

rspurgeon left a comment

Choose a reason for hiding this comment

mikefero left a comment

Choose a reason for hiding this comment

rspurgeon commented Sep 12, 2023

Tieske commented Oct 17, 2023

Tieske commented Oct 17, 2023

rspurgeon commented Oct 17, 2023

mheap commented Jul 24, 2023 •

edited by Tieske

Loading

CLAassistant commented Jul 24, 2023 •

edited

Loading

GGabriele Aug 23, 2023 •

edited

Loading

codecov-commenter commented Aug 22, 2023 •

edited

Loading