Allow users to ignore specific built-in rules by name

[viswajithiii]

I would like to be able to specify which built-in rules to ignore on the command line. Currently, the closest mechanism that exists is the strict flag, but the rules that disables aren't the ones I would like to disable. (I'm facing a situation where I want to prevent proto message breakages, but I'm okay with RPC compatibility breakages.)

If you agree this would be good to have, I'm happy to send a PR for this your way. 🙂

[nilslice]

Hey @viswajithiii -

Before considering the PR you've suggested, would you be open to trying a comment annotation on your RPCs? Currently, the @protolock:skip hint will ignore a message from being saved to the lock file, and thus is ignored when comparing versions.

Here's an example:

protolock/testdata/test.proto

Lines 55 to 56 in c6b4295

	// @protolock:skip
	message NextRequest {}

(here, the message NextRequest {} is not added to the lock file at all).

The comment (called a "hint" in protolock) is not implemented on any other type aside from message, and I think this would be a better PR, at least initially, since it may satisfy your need, and still abide by the existing use of the hint in protolock today. It would be great if a PR would cover the implementation of these hints on all the supported types.

My main concern with a configurable base of built-in rules, is that depending on how the configuration is stated, a user may not be aware of ignored rules, and it could break API compatibility.

I see value in making the tool more flexible, though, and appreciate this idea!

[viswajithiii]

Hey @nilslice, thanks for your response! Thanks for the suggestion about @protolock: skip -- I wasn't aware of this piece of functionality, and it's helpful. However, it doesn't solve this specific problem -- I think having to put this annotation above every RPC would be inconvenient and unnecessarily pollute our proto files.

Further, this is not the only use case I have in mind for this. Another example of a thing I would like to do is to have a rule that doesn't require field names to be reserved, just ids. I'm not sure there's a great way to achieve this with the tool right now.

I do understand your concerns about this creating a whole new surface of API compatibility that would have to be preserved. For now, I've created a fork and made the changes I need in a (destructive) way and I'm using that for now. I'll try to spend some time thinking of how best to achieve this within the constraints of this repo, though.

[nilslice]

@viswajithiii - I wanted to follow up on this, since I am starting to think about supporting an optional configuration file for protolock. Within this conf, it would be possible to disable any of the built-in rules (among other features).

I was wondering if you had come up with a solution for your initial issue or if you have some feedback/suggestions for a conf file? Thanks!

See #86 for some more info as this progresses.

[wadejensen]

Would also be interested in being able to enable/disable rules by name.

Use case:

We represent schemas for analytics data sent as JSON, with proto schemas. Since analytics event data must maintain backwards compatibility with historical events, removing fields, even when marking them as reserved is not allowed. If a field needs to be removed, then really it should be in an entirely new event in our system.

I'd like to disable the No Removing Fields Without Reserve check and replace it by introducing a plugin of my own which just says "no removing new fields, period", so my users are not presented with two (slightly contradictory) error messages.

But right now the only way to accomplish that is to wrap protolock and expressly filter out any undesired error messages with a hacky regex, and also use my custom plugin.

[viswajithiii]

@nilslice I apologize, I completely missed your previous note, but got notified now that @wadejensen commented. An optional config file sounds like a great way to go about this -- one that is flexible, and doesn't break backwards compatibility. I'd be happy to share my thoughts on any design you come up with, if that would be helpful. 🙂

[nilslice]

@wadejensen, @viswajithiii -

I will revisit this over the next few days and propose a config design/implementation. Will post back here with updates. Thanks!

[guyisra]

@nilslice any updates?

[nilslice]

@guyisra I suppose a minimal, extensible option would be to read a --config path and only support a single config option for the time being...

# protolock.yaml
skip_rules: 
    - NoUsingReservedFields
    - NoRemovingReservedFields

And provide this config to the rule checking step (or create a CompareWithConfig func?), which has knowledge of the name of each rule as it is being run:

protolock/parse.go

Lines 474 to 492 in c61398f

	for _, rule := range Rules {
	wg.Add(1)
	go func() {
	if debug {
	beginRuleDebug(rule.Name)
	}
	_warnings, _ := rule.Func(current, update)
	for i := range _warnings {
	_warnings[i].RuleName = rule.Name
	}
	if debug {
	concludeRuleDebug(rule.Name, _warnings)
	}
	warnings = append(warnings, _warnings...)
	wg.Done()
	}()
	wg.Wait()
	}

I don't have the time currently to implement and test, but would be happy to review a PR if you need this functionality and would like to take a stab it it!