Updating layer validations

[perryqh]

Layer Validation rules

• if a layer is specified in a package.yml, it must be one of the layers defined in packwerk.yml
• if enforce_architecture is true or strict, a layer must be specified
• a packages' dependencies should not be used in layer validations. check will find layer violations

[alexevanczuk]

I think we still want the architecture validator to ensure that stated dependencies do not violate the architecture right?

Is your point that if there is a reference that the checker finds, then it's the reference that violates architecture – not the stated dependencies? And we already have that behavior in the system?

My only thought here is that even if there are zero references, it's still wrong to have a dependency that conceptually violates a checker that should be restricting that dependency from existing. Can you share more about why you feel like this should be removed?

[perryqh]

Thanks for the feedback! There's a few concerns we have about one type of validation using another type's package.yml configuration.

1. Maintaining separation between dependency definitions and architectural layer validations is beneficial. This separation allows teams to selectively apply different types of validations without worrying about interdependencies between them.

2. If layer validation takes into account declared dependencies, the following scenario will come up:
   i) A package.yml specifies a legitimate dependency
   ii) Subsequently, a new layer validation is introduced involving the valid dependency
   iii) If layer validation flags the dependency as invalid, the dependency will need to be removed and both layer and dependency violations will be added to the associated package_todo.yml

3. Layer validation against unnecessary dependencies is of limited value. If we're concerned about unnecessary dependencies, we could consider making them dependency validation errors.

[alexevanczuk]

I'm fine with keeping what you have, but here's what I think:

1. I see things slightly differently here. I don't consider the dependencies key to be a property/definition of the dependency checker. Instead, I see dependencies as being part of the fundamental properties of a pack, and both the dependency checker and the architectural checker can apply their independent constraints on top of that shared concept.
2. This almost seems like a good thing to me and is conceptually correct – the new architecture constraint would dictate that the dependency is no longer permitted. If the user wants to ignore the subsequent dependency violations associated, they can add the dependency to ignored_dependencies.
3. Agreed, it doesn't add much value. I think it's nice in so far as someone can trust that when they look at a package dependencies, it should never violate the architecture.

Ultimately though no big deal... we can try this out and iterate!

[alexevanczuk]

This makes sense... if a pack enforces architecture, it must specify a layer. Surprised we didn't have this before!

[alexevanczuk]

Huh yeah, I double checked and it definitely just let it fly before!

[alexevanczuk]

So we do have this right, but it was just a bail vs. a first-class error right?

(Related to error message: Could not find one of layer {}or layer{}inpackwerk.yml...)

[perryqh]

Right. The referenced code above is an architecture layer validation message. The "could not..." is a "check" error.

[alexevanczuk]

Looks good @perryqh but I had some questions. I still think we should validate dependencies do not contradict the architecture rules.

I can see the argument that if:

1. Unnecessary dependencies are caught by the unnecessary dependencies check
2. If it's not unnecessary, the reference checking will find the violation

I just think it makes the constraints of the architecture checker slightly more coupled to using the unnecessary dependencies to be fully valid. Right now, that's at the expense of potentially slightly duplicate feedback (i.e. the user sees check AND validate fail).

However, I think packs should move toward merging check and validate into one call. IMO they are conceptually identical from a UX POV and the only reason they were separate was because packwerk was originally too slow to do them both at the same time. However, I don't see why they couldn't just be a single call, what do you think? If we had that, then the architecture checker can de-duplicate its messages before delivering them to the user.

[perryqh]

Looks good @perryqh but I had some questions. I still think we should validate dependencies do not contradict the architecture rules.

I can see the argument that if:

1. Unnecessary dependencies are caught by the unnecessary dependencies check
2. If it's not unnecessary, the reference checking will find the violation

I just think it makes the constraints of the architecture checker slightly more coupled to using the unnecessary dependencies to be fully valid. Right now, that's at the expense of potentially slightly duplicate feedback (i.e. the user sees check AND validate fail).

However, I think packs should move toward merging check and validate into one call. IMO they are conceptually identical from a UX POV and the only reason they were separate was because packwerk was originally too slow to do them both at the same time. However, I don't see why they couldn't just be a single call, what do you think? If we had that, then the architecture checker can de-duplicate its messages before delivering them to the user.

Agree that combining check and validate could be beneficial. Maybe we could support new options to validate? pks validate --include-check or something similar.

[alexevanczuk]

Agree that combining check and validate could be beneficial. Maybe we could support new options to validate? pks validate --include-check or something similar.

Yeah I'm not sure 🤔 I think any system/person that is using packs in a codebase that uses packwerk definitely wants it to give them the correct set of things that are correct and incorrect, but I think as a user I'd prefer if check just did everything and if I want packs to not validate, it should be the other way around, e.g. --no-packwerk-validate or something that shims the checks that relate to validate in packwerk OUT of packs.

But not something we need to do right now :)

[alexevanczuk]

@perryqh Merged this PR.

I don't 100% agree with not having the architecture checker check the list of dependencies, but since that only causes a behavioral difference when there are no references (otherwise it would result in an architecture violation) and there's the "unnecessary violations check" as a stop gap, it's not that important to me.