In strict mode, check only for violations unlisted in package_todo.yml

[alxckn]

What are you trying to accomplish?

Following #367, this PR changes the behavior of strict mode to only consider unlisted violations: only new violations, unlisted in the package_todo.yml file will result in an error when using the 'check' command.

Type of Change

• Bugfix
• New feature
• Non-breaking change (a change that doesn't alter functionality - i.e., code refactor, configs, etc.)

Additional Release Notes

• Breaking change (fix or feature that would cause existing functionality to change)

Organization relying on the previous definition of strict mode would need to be warned.

Checklist

• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• It is safe to rollback this change.

[rafaelfranca]

I'm not sure having a configurable mode is necessary. Let's just make that the default behavior of strict. If you have recorded violations and you enable strict mode, those should be fine, but new violations aren't. If you don't have any recorded violation and you enable strict mode same thing apply.

[alxckn]

@rafaelfranca ok. In your opinion, is it still the Checker's role to determine whether a violation is strict or not depending on whether it was listed or not in the package_todo.yml?

[alexevanczuk]

@alxckn

So the strict refers to the checker's level of enforcement, not the violation itself.

When the level is "true," we fail check on unlisted violations already and also permit new violations to be added.

When the level is "strict," we do not permit any violations ever.

It sounds like you want something in the middle -- allow listed violations, but no new ones.

I think @rafaelfranca 's suggestion makes sense. The only downside is now strict is less strict than it used to be -- a package with strict enforcement may have violations (or more easily regress to having them), and there's no mode to say "violation free." Upside is "strict" mode becomes a lot more useful to stop the bleeding. Curious what others think!

[alxckn]

@rafaelfranca @alexevanczuk

My proposal was to avoid having too big of a breaking change. If organizations rely on the previous definition of 'strict' to function then it might be a complicated upgrade.

Keeping two distinct modes could have upsides, for instance not all violations for a given package are recorded within its package_todo.yml (I'm thinking of the privacy checker). Maintaining a 'zero violation policy' (existing strict mode behavior) with packwerk only enforcing a 'no new violation' policy could become a challenge for package owners as they don't own the file where the violations are being written. This may be a separate issue though.

strict refers to the checker's level of enforcement, not the violation itself

I believe this can be true if strict mode only applies to unlisted violations. If we wanted to support two different modes such as 'strict' and 'strict_for_new', then it's the violation that is either strict or not depending on the package configuration (source or destination) and the listed status of the violation.

In any case I've updated the PR to reflect this direction, i.e. the check command may only fail because of strict mode violations if they're coming from unlisted offenses.

[alxckn]

@rafaelfranca @alexevanczuk I'm available to continue the discussion or amend the PR as needed :)

I did make the change to support the behavior suggested by Rafael, ie. change the existing strict mode to not account for violations that are already present in the package_todo.yml file.

[oboxodo]

I suggest keeping strict mode as it is: really strict. But add a new stop_the_bleeding mode (I like that name but you pick whatever).

This stop_the_bleeding mode can be implemented in two ways:
A. As this PR suggests for the strict mode, where existing violations would be allowed but new ones would fail.
B. Just focus on the total count of violations of that kind.

I think an approach like B might probably be simpler to implement but most importantly it wouldn't be a problem when we're just moving a file from one package to another or stuff like that. In that situation, an implementation like A will force us to change the mode to true to accept the moved files then back to strict.

I think the most important thing here is to avoid increasing the number of dependency/privacy/etc violations. If we add a new violation but solve one, even if it's a really new one... well... at least we're violations-neutral (like carbon-neutral, 😅).

[oboxodo]

Other possible names for the new mode could be: not_more, not_worse, healing, flexible...

[alxckn]

@oboxodo I had initially proposed a strict_for_new_mode in this PR (see first commit).
There are also good arguments for changing the behavior of the existing strict mode, among which:

• simplicity, avoid opening the door to new modes that bring new subtleties each time
• it does not change much for existing packages that are already using strict mode, no new violation is allowed

[oboxodo]

@alxckn ah, ok. I hadn't seen that. I agree on both points.

Anyway, I think any of the proposed solutions would be better than the current strict mode.

[rafaelfranca]

Let's just change the strict mode. It means no new violation. If you have 0 violation (the current implementation) nothing changed for you. If you have violations, now you can enable strict mode.