#60 / #63 Resolving package cycle and setting up Deptective #62
Conversation
|
Note this also reduces the dependencies from |
|
Actually, this is growing on me. I think it's a step into the right direction. @tombentley, feel free to merge it, if you're happy with it, too. |
gunnarmorling
changed the title
| @@ -263,5 +263,39 @@ | |||
| </plugins> | |||
| </build> | |||
| </profile> | |||
| <profile> | |||
| <id>architecture-check</id> | |||
There was a problem hiding this comment.
The check is actually very fast, so it could be part of the "qe" profile and just always run. I've kept it separate for two reasons:
- There's no stable Deptective release available on Maven Central yet (working on this atm.), pulling it via JitPack for now
- Deptective currently doesn't work with Java 17; while we agreed on not moving to 17 for the time being (as in actively using its features), I don't want to give up on the capability to run the build with 17
|
|
||
| import io.netty.buffer.ByteBuf; | ||
|
|
||
| public interface ByteBufAccessor extends Writable { |
There was a problem hiding this comment.
Can we document what it's for?
| @@ -22,10 +22,11 @@ | |||
| * depends on NIO ByteBuffer, so copying between ByteBuffer and ByteBuf cannot | |||
| * always be avoided. | |||
| */ | |||
| public class ByteBufAccessor implements Readable, Writable { | |||
| public class ByteBufAccessorImpl implements ByteBufAccessor, Readable { | |||
There was a problem hiding this comment.
Why do we have an asymmetry here between the Readable and Writable now?
There was a problem hiding this comment.
Because clients of ByteBufAccessor currently only access the Writable methods, so I limited what it exposes (following my general preference for being restrictive in terms of what gets exposed). Do you see a need for exposing Readable there as well?
| "name" : "io.kroxylicious.proxy", | ||
| "contains" : [ "io.kroxylicious.proxy" ], | ||
| "reads" : [ "io.kroxylicious.proxy.filter", "io.kroxylicious.proxy.internal", "io.kroxylicious.proxy.internal.filter", "io.netty.*", "org.apache.logging.log4j" ] |
There was a problem hiding this comment.
To confirm I understand this:
nameis a logical name for this "rule"?containsis a package name or pattern for matching packages?readsis basically "is allowed to import/use from these other packages"? And may be package names or patterns
There was a problem hiding this comment.
Yes and yes to the first two. On reads, it's references to other "components" (what you referred to as "rules"). not packages.
| <showWarnings>true</showWarnings> | ||
| <compilerArgs> | ||
| <!-- <arg>-Xplugin:Deptective mode=ANALYZE components=io.netty.*:io.netty*;org.apache.kafka.*:org.apache.kafka*;io.kroxylicious.proxy.example.*:io.kroxylicious.proxy.example* visualize=true</arg> --> | ||
| <arg>-Xplugin:Deptective mode=VALIDATE cycle_reporting_policy=ERROR reporting_policy=WARN visualize=true</arg> |
There was a problem hiding this comment.
So I guess all I really want to know is:
- Will this fail the CI when the rules are broken?
- Will the report contain the cute graph visualisation?
There was a problem hiding this comment.
No and no, as of now. Although ideally I'd want that to change to yes and yes eventually. The former will be easy, just a matter of running the build with that profile. On the latter, I was discussing it with @aalmiray (my partner in crime working on ModiTect), the following would have to happen:
- Have Deptective produce Mermaid syntax rather than dot file (GraphViz)
- Have a GH bot which takes the Mermaid stuff from a defined file in the workspace and pushes a comment with it
The latter is organizationally challenging, as we'll need a runtime environment for that bot. Unfortunately, regular GH Actions cannot push comments to PRs.
In any case, I'd leave all that to a follow-up issue.
…oid circular package dependency
|
@tombentley, any more concerns about this one? |
Fixes #60.
Fixes #63.