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

Allow to filter runners via config #371

Merged

Conversation

povilasjurcys
Copy link
Contributor

@povilasjurcys povilasjurcys commented Feb 25, 2020

This feature allows to specify which runners will be triggered during pronto run. Might solve #365.

Reason

I use pronto:

  • on local machine
  • on CI
  • as git pre-commit hook
  • as git pre-push hook

In each of those cases I would like to run different linters. For example I want to run all pronto linters on CI and only fast-enough linters on git pre-commit hook. With this feature I could add PRONTO_SKIP_RUNNERS or PRONTO_RUNNERS env variable in order to control which runners will be triggered.

I know that there is a -r (aka --runner) flag, but:

  • it does not have --skip-runner options;
  • it does not work very well, especially with other flags (some times it tries to load file pronto/-r file);
  • it does not skip runners which where already loaded.

Filtering

Whiteliting runners via config

# .pronto.yml
runners:
  - rubocop
  - reek

This will ensure that only rubocop and reek runners will run.

Blacklisting runners via config

also it's possible to blacklist runners (filter out) like this:

# .pronto.yml
skip_runners:
  - rubocop
  - reek

Filtering via env variable

Filtering can be done with env variables:

# whitelist:
PRONTO_RUNNERS="rubocop reek" pronto run

# blacklist:
PRONTO_SKIP_RUNNERS="rubocop reek" pronto run

Other notable things

I could not resist to do small refactoring in config.rb and runners.rb classes, I hope it's not a big deal. Let me know if I need to open separate ticket just for refactoring.

Also I'm not sure if config structure is good enough. As an alternative I see nested structure for runners like this:

runners:
  only:
    - reek
    - rubocop
  except:
    - rubocop

Would be nice to receive some feedback about it.

@povilasjurcys povilasjurcys requested a review from a team as a code owner February 25, 2020 05:08
@povilasjurcys
Copy link
Contributor Author

@doomspork can you check my PR?

Copy link
Contributor

@mknapik mknapik left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!
Skipping a runner for a git hook sounds like a valid use case.

Personally, I like a nested config more.

I'd love to see tests with a runners context with clear 4 cases:

  • no only nor except
  • just only
  • just except
  • both only and except

.gitignore Outdated Show resolved Hide resolved
lib/pronto/config.rb Outdated Show resolved Hide resolved
lib/pronto/runners.rb Show resolved Hide resolved
lib/pronto/config.rb Outdated Show resolved Hide resolved
@povilasjurcys
Copy link
Contributor Author

Personally, I like a nested config more.

@mknapik are you talking about this case:

# .pronto.yml
skip_runners:
  - rubocop
  - reek

or something completely different?

I'd love to see tests with a runners context with clear 4 cases:

  • no only nor except
  • just only
  • just except
  • both only and except

Added more tests. Let me know if there is something else missing.

@mknapik
Copy link
Contributor

mknapik commented Mar 31, 2020

You've proposed a structure like:

Also I'm not sure if config structure is good enough. As an alternative I see nested structure for runners like this:

runners:
 only:
   - reek
   - rubocop
 except:
   - rubocop

and I think I like it more.
It would also probably require adjusting env vars accordingly (probably PRONTO_RUNNERS_EXCEPT and PRONTO_RUNNERS_ONLY).
@prontolabs/core Does anyone has on opinion on config format?

Copy link
Contributor

@mknapik mknapik left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes!
Let's wait for other team members @prontolabs/core to weight in on the YML format.

.gitignore Outdated
@@ -4,5 +4,4 @@ pkg/*
.DS_Store
Gemfile.lock
coverage
.history/
.vscode/
Copy link
Contributor

Choose a reason for hiding this comment

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

You've missed .vscode (you could amend the commit if you want to)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry about that. Fixed and squashed commits

@@ -133,14 +133,33 @@ module Pronto
describe '#skip_runners' do
subject { config.skip_runners }

context 'when there is an entry in the config file' do
let(:env_variables) { {} }
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@povilasjurcys povilasjurcys force-pushed the allow_to_filter_runners_via_config branch from b88710d to 8e27aee Compare March 31, 2020 09:52
@ashkulz
Copy link
Member

ashkulz commented Feb 18, 2021

@povilasjurcys can you rebase this PR?

@povilasjurcys
Copy link
Contributor Author

@ashkulz rebased the code 👍

@ashkulz
Copy link
Member

ashkulz commented Feb 19, 2021

Hmm, not sure why the CI ran twice and failed for pull_request -- do you have any idea, @dsander?

@dsander
Copy link
Contributor

dsander commented Feb 19, 2021

@ashkulz Not super sure to be honest, maybe #398 fixes that. Generally I think it was wrong to use pull_request_target in the action.

@ashkulz
Copy link
Member

ashkulz commented Feb 19, 2021

@povilasjurcys can I trouble you to merge/rebase again? GitHub CI failed for 2.4, hopefully #398 fixed that 🤞

@ashkulz
Copy link
Member

ashkulz commented Feb 19, 2021

Also, I'd appreciate a change to README.md which documents this change as well 🙂

@povilasjurcys
Copy link
Contributor Author

Also, I'd appreciate a change to README.md which documents this change as well

@ashkulz I added two lines in the readme. Let me know if that's enough or not.

can I trouble you to merge/rebase again? GitHub CI failed for 2.4, hopefully #398 fixed that crossed_fingers

Sadly tests for ruby 2.4 is still failing. I highly doubt that it's because of changes in this PR

@ashkulz
Copy link
Member

ashkulz commented Mar 18, 2021

I added two lines in the readme. Let me know if that's enough or not.

I think you need to elaborate what happens by default i.e. both the options are not provided, otherwise looks good to me!

Sadly tests for ruby 2.4 is still failing. I highly doubt that it's because of changes in this PR

Sorry to trouble you -- can you merge master again? @dsander made a PR which should fix that.

@povilasjurcys
Copy link
Contributor Author

I think you need to elaborate what happens by default i.e. both the options are not provided, otherwise looks good to me!

Added separate table with some top-level properties here. Since it's a new thing, I would like to hear your opinion

@povilasjurcys
Copy link
Contributor Author

povilasjurcys commented May 14, 2021

@ashkulz can you merge this PR?

@jarl-dk
Copy link

jarl-dk commented Aug 19, 2021

@ashkulz: It seems like this one can be merged now, right?

@ashkulz ashkulz merged commit 6b3290e into prontolabs:master Sep 6, 2021
@ashkulz
Copy link
Member

ashkulz commented Sep 6, 2021

Thanks for the contribution, @povilasjurcys! Sorry for the delay in reviewing 🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants