RFC: Change default behavior of opt-out to opt-in for repos and org-wide rulesets about peril-settings HOT 14 CLOSED

I'm a little confused by what you want from this RFC vs how it was built.

Right now, any PR to any repo gets:

• Assignment checks
• Spell checks for *.md
• Checks that there is a PR body
• If there's a changelog then it checks if it's been changes if there's code edits
• Mapping of commit prefixes to labels

Then any project opts in by adding their own Dangerfile in their repo + CI, e.g. volt/gravity/eigen/emission which has project specific rules.

Which feels to me like what you're wanting to address, can you explain how you'd want to see it differently?

from peril-settings.

Can we group the rules that are applied to "any PR to any repo" into an artsy.org namespace... then projects include those rules with rules.arsy.org.*?

from peril-settings.

We can make the "all pr" rules be opt-in yep, but I'd be a strong rejection for this RFC in that case.

The vast majority of our repos share the same culture, have a generally similar workflow and all share the same dev team. It makes sense for config or non-engineering repos to opt out, they're the rarity though.

from peril-settings.

I feel like Artsy at this point is only going to grow and getting things consistent at an organizational level is important, which includes having Peril there in the background. So for the rule change from opt-out to opt-in I'm 👎. I think over time our rule-sets will be refined to reflect the needs of the larger engineering organization and we should keep the train on its tracks. That said, being globally pestered about things like spelling, body content and CHANGELOG updates I think might be overstepping things a bit as these requirements don't quite capture the essence of what a global requirement should be.

In terms of spelling, since our PRs aren't blog-posts and are largely meant for internal consumption (and I'm confident all of our devs can spell, even if sometimes they cant) it seems a bit heavy handed and perfectionistic to warn against mistakes after every change. Some things can just be let be without much harm. Also, all of the not-in-dictionary abbreviations / terminology we use day to day is often flagged requiring context-switching to update the dictionary in another repo.

I just found a good example of this:

Some people don't mind updating this, while others most definitely do -- meaning, there's inconsistency introduced based on the assumption that all PR copy has to be spelling free, and what constitutes a spelling error (e.g., "writeup vs write-up"). As a repo-owner its totally cool to insist on this level of correctness, but extended engineering-wide it gets super edge-casy pretty quick.

And body copy, in many cases, isn't necessary -- whether to include it or not is largely contextual, and people generally do add body copy; when they don't its usually intentional (quickfix, etc). Changelogs, while opt-in due to the presence of a file, in many cases are just git / release-tag histories. Does every PR need a notice to update? Changelogs, too, seem contextual and cultural and should be scoped to a project-level Dangerfile. (For example, a lot of bugfix PRs don't necessarily need a changelog entry -- but when they do, that's largely due to the repo-owner / leads requirement). This requirement, too, is inconsistent globally, but when scoped locally makes perfect sense.

In sum, it can get annoying when constantly ignoring warnings from Danger due to repo-level cultures, and so global rules should very much be distilled down to the lowest-common-denominator by removing as much subjectivity as possible. Things that must be there should be there ("every PR must have a reviewer", for example); and things that can slide, or are disputable, should be avoided. (Something I just thought of is, easily discernible global rules can be thought of as subcommands for future tooling.)

from peril-settings.

Yeah, I've been mulling this over and I think I'm a 👎 too. If I understand this RFP correctly, what's under discussion is not the actual global rules, but the fact that we have global rules and that those rules are on by default.

I could possibly be persuaded to support an RFP for removing a global rule like the CHANGELOG check, but if we're going to have global rules, I believe they should be on by default. As @damassi says, these global rules can be annoying and thus the bar should be high. But once we've agreed on these global rules, on by default makes the most sense to me.

from peril-settings.

Thanks for the feedback - I do agree with many points raised here. @jonallured you put it quite well, "if we're going to have global rules, I believe they should be on by default". And that is why I added the exception above, in that I think assigning someone to a PR is a good global rule to have applied org-wide in terms of a Github workflow.

I echo what @damassi raised about spell checking. Thanks for digging up a specific example of this.

I do disagree with @orta your point that "our repos share the same culture". I think this is perhaps an over-generalization. When it comes to our Github workflow, yep I am with you on this. It's a practical concern to have someone assigned a PR and it not just being left hanging open. And perhaps this comes generally from an open-source culture / workflow. I would like to be specific here about what we mean by culture. I don't come from a predominantly open-source via Github background where I am used to working on small agile dev teams, sometimes using closed, self-managed repos and having to ping my colleagues to review my work is something I am used to, so I think that may be the source of my general confusion.

So perhaps a better way to rewrite or rephrase this (or open a new issue in Danger / new RFC) would be to keep global rules as-is, keep specific project rules as-is, but introduce a mechanism to import rules that are defined but not necessarily globally applied on the org-level. Then we would not have to remove global style rules like the CHANGELOG or spelling checks, but they could live in a place that would be defined org-wide, and imported by specific projects. See my second paragraph Additionally or alternatively... Would that address all of your concerns?

from peril-settings.

@orta also I think this is why I was unclear on whether this could be addressed by the RFC process in the first place. It's more pertaining to Danger. I can't find any documentation about whether this functionality might be available already via a plugin? It seems rules can only be defined globally or locally. Is this correct?

from peril-settings.

Great points from @jonallured and @damassi here... I agree with most of them. I too think that there is some space for global rules to be applied, such as PR assignment, and I also want Danger there to notify me of build failures, so i'm a 👎 on this RFC.

I would support an RFC that proposed an override (disabling) of global rules, specifically for your project, in a DangerFile. There can be an explicit reason why, for example, the 'Big PR' rule might not be meaningful in some contexts, but you may want to keep some other global Danger rules (such as PR assignation) without opting the entire repo out of Danger altogether.

from peril-settings.

@cavvia I like the idea of overriding / disabling global rules. I'm not sure how that would work in practice, for as far as I can tell these is no API for evaluating whether or not to run an RFC. I am guessing it would have to be implemented here somehow? Or perhaps the Danger API would need to support it. In any case it would require a way to refer to a given, named RFC in a Dangerfile. This is somewhat what I was getting at with defining rulesets. Another way to think about it would be using namespaces.

from peril-settings.

Global rules are completely separate from local rules, so if you pr'd to this repo. A lot of this discussion is quite abstract, but it's all code, so:

import spellcheck from "danger-plugin-spellcheck"
rfc("Keep our Markdown documents awesome", async () => {
+ if (danger.pr.author.login == "izakp") { return }
  await spellcheck({ settings: "artsy/[email protected]" })
})

to here

https://github.com/artsy/artsy-danger/blob/428787b3521bb26bb0fd2dbc08144887b005cf58/org/all-prs.ts#L22-L25

Then you'd never have spell checks done to markdown files in your PRs, but if you sent a PR to gravity, you'd still end up with a check for a big PR because that is the local danger which has a completely separate ruleset.

Personally, I'd rather not start chopping up which rules get applied to each repo. If you have a repo which is an edge case, you can opt out entirely, but I don't think we need the complexity of that for 5 rules we have that apply globally

from peril-settings.

@orta thank for the clarification. As far as I can tell Danger is a DSL for writing rules but doesn't have any API or abstractions governing how these rules are applied... is that a correct understanding of what you mean by "it's all code"? I suppose that this relates to Danger core and not really relevant to an RFC. I wouldn't want to write Dangerfiles with lots of branching if/else statements like this - it could get messy, as you say chopping up which rules get applied to which repo, PR author, etc...

from peril-settings.

Yep: http://danger.systems/js/

from peril-settings.

@orta after looking through DangerJS's source code I found the answer I was looking for, and my confusion stems from the fact that Dangerfiles are essentially executed via a NodeJS VM sandbox](https://github.com/patriksimek/vm2). It would be good to make some sort of reference to this in the documentation, as most of the docs explain how to use Danger but not much about its internals.

In light of this, I see that the changes proposed in this RFC are not possible without significant changes to Danger's execution model, and I so I will close this and make a feature request / suggestion for extending Danger's interfaces / capabilities in its own repo.

from peril-settings.

Feature request for Danger-JS submitted here: danger/danger-js#498

from peril-settings.