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

Make all pieces of loading/linting optional #26

Open
tarkatronic opened this issue Feb 22, 2021 · 4 comments
Open

Make all pieces of loading/linting optional #26

tarkatronic opened this issue Feb 22, 2021 · 4 comments
Labels
checkers Related to one or more of the checkers enhancement New feature or request fixers Related to one or more of the fixers help wanted Extra attention is needed loaders Related to one or more of the loaders

Comments

@tarkatronic
Copy link
Contributor

In the fashion of the linters I am familiar with, we should only check what is provided/requested, and not require a full check of all things every time. What I mean is, this should technically be considered a working config file:

[org]
login = "godaddy"

It wouldn't actually check anything, except that the org exists and the token used has access to it. But it would technically work.

Also important is that the above config SHOULD NOT load any additional info. It should not load membership data, team data, application data, org settings, etc. Just the login. Nor should it call any of the checkers.

Likewise, this should be a valid config:

[org]
login = "godaddy"

[[applications]]
appId = 2740
appSlug = "renovate"
repositorySelection = "selected"
events = [
  "check_run",
  "issues",
  "issue_comment",
  "pull_request",
  "push",
  "repository",
  "status"
]

[applications.permissions]
checks = "write"
issues = "write"
members = "read"
contents = "write"
metadata = "read"
statuses = "write"
workflows = "write"
pull_requests = "write"
administration = "read"
vulnerability_alerts = "read"

The above config would load ONLY the org login name, and application installations. It would then verify that ONLY the "renovate" application is installed, and that its installation matches the config.

So in other words, if a config section exists, it triggers the appropriate loaders/checkers/fixers. If that section does not exist, those are not loaded.

@tarkatronic tarkatronic added enhancement New feature or request help wanted Extra attention is needed checkers Related to one or more of the checkers fixers Related to one or more of the fixers loaders Related to one or more of the loaders labels Feb 22, 2021
@msluther
Copy link
Contributor

should it work that way or should it have a default rule set that you can use your config file to opt-in/-out of additional rules. e.g. maybe it runs what you currently have by default and then you can do something like

[settings]
rules = ['some', 'list', 'of', 'rules']

I feel like trying to be smart about what is in the settings file makes it easy to accidentally think its fixing more than what its actually doing (e.g. "oops I misspelled applications" so it did nothing).

@tarkatronic
Copy link
Contributor Author

Interesting thought. So maybe there could be an [orglinter] specific section, something like:

[orglinter]
# Explicit set of _included_ checkers
enable = ["org", "applications"]

# OR explicit list of _excluded_ checkers
disable = ["members", "teams"]

That would also allow for more tool-specific configuration later on down the line. And we can be very explicit about allowed keys and values in this section.

@msluther
Copy link
Contributor

Exactly, it allows the tool to be opinionated about what you should check without forcing that. I also think you might want to allow for CLI overrides of those as well (so a hierarchy of CLI >> my.toml >> defaults). that way you can do one-off checks without having to modify a source-control'd config file (e.g. I just want to check for demotions real quick)

@tarkatronic
Copy link
Contributor Author

Definitely agreed on the priority order. We might even toss env vars in there just for completeness. CLI > env > my.toml > defaults

But that all depends on actually setting up a proper CLI app first. I think I've mentioned that in a couple issues now. That should probably be encapsulated in its own issue... because it seems that it's becoming a blocker for a number of things now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
checkers Related to one or more of the checkers enhancement New feature or request fixers Related to one or more of the fixers help wanted Extra attention is needed loaders Related to one or more of the loaders
Projects
None yet
Development

No branches or pull requests

2 participants