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 new command inventory lint #400

Merged
merged 5 commits into from
Jan 21, 2022
Merged

Add new command inventory lint #400

merged 5 commits into from
Jan 21, 2022

Conversation

simu
Copy link
Member

@simu simu commented Jan 19, 2022 •
edited
Loading

Currently the only implemented linting error is warning about component specifications without explicit version.

Checklist

  • Keep pull requests small so they can be easily reviewed.
  • Update the documentation.
  • Update tests.
  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking, dependency
    as they show up in the changelog

@simu simu added the enhancement New feature or request label Jan 19, 2022
simu added 3 commits January 19, 2022 17:15
@simu
Add new command inventory lint components
8f94244 
Currently the only implemented linting error is warning about component
specifications without explicit version.
@simu
Add tests
b034fa7
@simu
Convert assertion into exception to make Bandit happy
ce969f3
@simu simu force-pushed the feat/lint-component-specs branch 2 times, most recently from d887245 to 644dbbf Compare January 19, 2022 16:23
@simu simu force-pushed the feat/lint-component-specs branch from 644dbbf to 60433c4 Compare January 19, 2022 16:25
@simu simu requested review from corvus-ch and glrf January 19, 2022 16:38
glrf
glrf approved these changes Jan 20, 2022
View reviewed changes
Copy link
Contributor

@glrf glrf left a comment

Choose a reason for hiding this comment

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

LGTM.
I would have structured the CLI differently, but that's probably personal preference,

commodore/cli.py Outdated
Comment on lines 427 to 428
def inventory_lint(config: Config, verbose):
config.update_verbosity(verbose)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make this command lint everything (i.e. execute all it's sub commands)?

I pretty much never want to just check for one type of issues. I would want commodore to just lint my inventory. I don't want to run a command to check if there are components without versions, then another one check for a different issue, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if that's possible with the current structure, but we can certainly refactor the CLI to just provide inventory lint and then options to enable or disable individual linters. Thinking about it again, that probably makes more sense than my current approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the CLI to just provide inventory lint. We can always add flags to enable or disable individual linters once we have more than one.

corvus-ch
corvus-ch reviewed Jan 20, 2022
View reviewed changes
Copy link
Contributor

@corvus-ch corvus-ch left a comment

Choose a reason for hiding this comment

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

I fail to understand where that linting would be executed.

@simu
Copy link
Member Author

simu commented Jan 20, 2022

I fail to understand where that linting would be executed.

Anywhere you can run Commodore really. This can be useful locally to check that you didn't introduce an unsupported configuration, or in CI for a global or tenant repo to catch unsupported configs before they get merged to the main branch.

@simu simu changed the title Add new command inventory lint components Add new command inventory lint Jan 20, 2022
@simu simu requested a review from glrf January 20, 2022 13:46
glrf
glrf reviewed Jan 20, 2022
View reviewed changes
Copy link
Contributor

@glrf glrf left a comment

Choose a reason for hiding this comment

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

Maybe I'm completely misunderstanding how this should work, but it seems like you're missing a flag declaration?

commodore/cli.py Outdated Show resolved Hide resolved
tests/test_cli.py Outdated Show resolved Hide resolved
@simu
Restructure CLI command
ca753d0 
Only provide `inventory lint`. We can introduce flags to control which
linters are enabled once we have more than one.
@simu simu force-pushed the feat/lint-component-specs branch from efc67a4 to ca753d0 Compare January 20, 2022 14:15
@simu simu merged commit 7f66b41 into master Jan 21, 2022
@simu simu deleted the feat/lint-component-specs branch January 21, 2022 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glrf glrf glrf approved these changes

@corvus-ch corvus-ch corvus-ch left review comments

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@simu @corvus-ch @glrf