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

⚙ Configuration #96

Closed
gr2m opened this issue Jun 15, 2018 · 13 comments · Fixed by #128
Closed

⚙ Configuration #96

gr2m opened this issue Jun 15, 2018 · 13 comments · Fixed by #128
Assignees
Labels

Comments

@gr2m
Copy link
Collaborator

gr2m commented Jun 15, 2018

I’m in the process of submitting the WIP app to the Marketplace. The new version will support configuration and I’d like to invite y’all to discuss what the configuration should look like. Please share your workflows and how WIP would ideally work to seamlessly integrate with it :)

Right now I see two things that should be configurable

  1. The terms that the WIP app will look for (by default "WIP", "Work in progress" and "Do not merge", case insensitive)
  2. The locations that the WIP app will check for the configured terms (by default only the pull request title)

The configuration will be done via a .github/wip.yml file in your repository. See https://github.com/probot/probot-config for how configuration works with Probot and stale app usage for an example

Example configuration

This is my base of discussion, please comment below if it that works for you or if you have any questions/suggestions

# Configuration for WIP - https://github.com/wip/app

# List of terms that will make WIP set a pull request status to "pending".
# All terms are work in case insensitive and will be matched as whole words,
# e.g. "WIP" will match "wip: refactoring" but not "Add swipe feature"
terms: 
- WIP
- Work in progress
- Do not merge

# List of places that WIP will check for the terms defined above
# Possible values are
# - title            (pull request title)
# - labels           (label names)
# - commit_subjects  (commit subjects)
check:
- title

Discussion

I’m not happy with the term "check". Maybe someone has a better idea?

@blairconrad
Copy link

@gr2m, I like the configuration file location.

The configuration looks good, and easy to understand. This is added complexity, but have you considered allowing the application of different terms to each check location? Probably more bother than it's worth…

Regarding this issue's relation to #55, I take it I could then configure like this to replicate the behaviour I was interested in?

terms:
- fixup!
- squash!

check:
- commits

Thanks!

@gr2m
Copy link
Collaborator Author

gr2m commented Jun 15, 2018

I think for the usecase to block PRs if there are commits with "fixup!" or "squash!" it works good enough to add them to the terms config. I don’t think there is a chance for false positives that these terms could be found in labels or the pull request title

@u2606
Copy link

u2606 commented Jun 15, 2018

As an alternate name for check, what do you think about locations? Then both configuration options would be nouns, not a noun and a verb.


The option commits is slightly unintuitive if it only refers to commit subjects. I don’t have a strong opinion between the following:

  • changing the option to also search commit bodies
  • renaming the option to something like commit_subjects
  • renaming the option, and also adding a commit_bodies option

My team’s workflow is to not allow merging if there’s a “hold” label, a “fixup!” or “squash!” commit, or “WIP” in the PR title or a commit. I’m not concerned about false positives for “fixup!”, “squash!”, or “WIP”, but I could see “hold” being used for unrelated reasons in commit titles. A potential syntax for configuration per check location could look like:

-
 terms:
   - hold
 locations:
   - label
-
 terms:
   - fixup!
   - squash!
 locations:
   - commits
-
 terms:
   - WIP
 locations:
   - title
   - commit

That is, each set of term/location pairs would be a list item.

@gr2m
Copy link
Collaborator Author

gr2m commented Jun 15, 2018

These are great suggestions, thank you! I particularly like the suggestion of supporting a list of configurations of terms/locations pairs 👍 I think that's simple enough to explain and implement

The option commits is slightly unintuitive if it only refers to commit subjects

I had the same thought. I can’t think of a use case where I would look for a term in a commit body, but I agree that "commit_title" is more clear than just "commit"

@gr2m
Copy link
Collaborator Author

gr2m commented Jun 28, 2018

it’s now implemented as part of #94

You can see it working at wip/sandbox#3, configuration at https://github.com/wip/sandbox/blob/8377a438bfed66888c8ba7f72945c80bcaa55cb2/.github/wip.yml

This was referenced Jun 30, 2018
@tomkerkhove
Copy link

tomkerkhove commented Jul 1, 2018

The configuration will be done via a .github/wip.yml file

Are there plans to also support .github/apps/wip.yml? Would be great to have a subfolder for apps so that GitHub configs are seperated.

@tomkerkhove
Copy link

  • Pricing - Based on https://github.com/wip/app/blob/marketplace-app/HOW_IT_WORKS.md I've noticed configurations are only supported for Pro. Did you consider configuration to be supported for Free as well but only for PR title? This would allow users to still configure the terms without having full power. Not a requirement for me but just thinking out loud.
  • Configuration Naming - I like location more than checks but maybe it's a bit too vague. How about check_locations or verification_locations?

@gr2m
Copy link
Collaborator Author

gr2m commented Jul 1, 2018

Are there plans to also support .github/apps/wip.yml

.github/wip.yml is the convention coming from Probot, I’ll stick to it

I've noticed configurations are only supported for Pro

Honestly if you need the configuration but can’t afford the $1/month (it will all be donated to https://railsgirlssummerofcode.org/) then I’m sure I can get you a voucher or something, don’t worry about it.

I like location more than checks but maybe it's a bit too vague. How about check_locations or verification_locations?

Thanks for your input! I think locations is fine now, I’ll make sure to document it properly

@gr2m
Copy link
Collaborator Author

gr2m commented Oct 14, 2018

Configuration is here 🎉

I’ve pushed it to the beta version of the WIP app and would love all your help testing it, please see my update in the 🤖📯 Updates issue.

Thank you all for your patience and help 🙏

@gr2m
Copy link
Collaborator Author

gr2m commented Oct 23, 2018

🎉 This issue has been resolved in version 3.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@gr2m gr2m added the released label Oct 23, 2018
@gr2m
Copy link
Collaborator Author

gr2m commented Oct 24, 2018

You can sign up for the pro plan on marketplace now to use the configuration feature https://github.com/marketplace/wip.

All revenue from the "pro" plan will be donated to Rails Girls Summer of Code. I only added the paid plan to make the WIP a real-life GitHub App example. If you cannot pay but depend on the pro features you can add your account with an explanation to the pro-plan-for-free.js file.

@alecmev
Copy link

alecmev commented Nov 7, 2018

Can WIP configuration defaults be changed for the whole organization, instead of on per-repo basis? If not, is it possible to implement?

@gr2m
Copy link
Collaborator Author

gr2m commented Nov 7, 2018

Not yet, but once this PR gets merged and released, you can create a .github repository and out the default configuration file for your user or org in there: probot/probot-config#16 (comment)

What you can do today is reference other config files, see docs at https://github.com/probot/probot-config/

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

Successfully merging a pull request may close this issue.

5 participants