-
Notifications
You must be signed in to change notification settings - Fork 693
ArchUnit rule to support both camelCase and kebab-case in ConditionalOnProperty #36872
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ArchUnit rule to support both camelCase and kebab-case in ConditionalOnProperty #36872
Conversation
0334c69
to
aaacddc
Compare
6f44a78
to
6da685a
Compare
Adds a new module that can be used to specify ArchUnit tests for the orchestration cluster. These tests have access to the entire production code through a dependency on dist (io.camunda:camunda-zeebe). The tests need to be run by surefire rather than failsafe to ensure correct execution and access to loaded classes. With failsafe, tests are executed but failures are not detected somehow. I did not understand why this is. Note that the parent module (io.camunda:camunda-qa) attempts to run all tests with failsafe rather than surefire so the archunit-tests's pom explicitly re-configures this. The choice to create a new module was made as it: - allows to easily run these tests together - depending on dist ensures it runs on all production code, including modules added later without needing to change dependencies - helps others to find a fitting place for their ArchUnit tests - groups together ArchUnit tests for easily gaining an overview The downsides are: - long feedback loop - I value this less than the long term ease of maintenance. - doesn't run on non-prod code - No need to solve this now. See: https://camunda.slack.com/archives/C08F5RD5X8B/p1755527617038109
The ConditionalOnProperty annotation requires kebab-case in its attributes in order to correctly match them using relaxed binding to spring loaded properties. In other words, to match to both my-example and myExample, the @ConditionalOnProperty must refer to my-example. If it refers to myExample, then it only works explicitly on myExample and relaxed binding to my-example doesn't work. To avoid this, we add an ArchUnit test that verifies that none of the attributes of ConditionalOnProperty use camelCase. It does this simply by checking for capitalized letters. If it fails, the error message includes all the needed details, like the location of the forbidden annotation, the specific attribute, and the failure. To overcome the problem, simply switch to using kebab-case in the attributes of ConditionalOnProperty.
ConditionalOnProperty requires specifying names of properties using kebab-case. This ensures that camunda.tasklist.webapp-enabled can be used rather than requiring camunda.tasklist.webappEnabled explicitly. Note that the docs document it as camunda.tasklist.webappEnabled, but this change is backward compatible, as camunda.tasklist.webappEnabled can still be matched due to relaxed binding. We decided to use kebab-case for configs in the docs, but they have not been adjusted yet.
ConditionalOnProperty requires specifying names of properties using kebab-case. This ensures that camunda.operate.webapp-enabled can be used rather than requiring camunda.operate.webappEnabled explicitly. Note that the docs document it as camunda.operate.webappEnabled, but this change is backward compatible, as camunda.operate.webappEnabled can still be matched due to relaxed binding. We decided to use kebab-case for configs in the docs, but they have not been adjusted yet.
ConditionalOnProperty requires specifying names of properties using kebab-case. This ensures that camunda.tasklist.importer-enabled can be used rather than requiring camunda.tasklist.importerEnabled explicitly. Note that the docs document it as camunda.tasklist.importerEnabled, but this change is backward compatible, as camunda.tasklist.importerEnabled can still be matched due to relaxed binding. We decided to use kebab-case for configs in the docs, but they have not been adjusted yet.
ConditionalOnProperty requires specifying names of properties using kebab-case. This ensures that camunda.operate.importer-enabled can be used rather than requiring camunda.operate.importerEnabled explicitly. Note that the docs document it as camunda.operate.importerEnabled, but this change is backward compatible, as camunda.operate.importerEnabled can still be matched due to relaxed binding. We decided to use kebab-case for configs in the docs, but they have not been adjusted yet.
ConditionalOnProperty requires specifying names of properties using kebab-case. This ensures that camunda.tasklist.feature-flag.process-public-endpoints can be used rather than requiring camunda.tasklist.featureFlag.processPublicEndpoints explicitly.
6da685a
to
eba1343
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @korthout ! 🚀
👍 I like having the new module, and I hope it will be used more often to include similar tests like this one.
❓ Do we need to do anything extra to have tests in this module run in the CI? I assume not?
...tests/src/test/java/io/camunda/archunit/RequireKebabCaseInConditionalOnPropertyArchTest.java
Outdated
Show resolved
Hide resolved
...tests/src/test/java/io/camunda/archunit/RequireKebabCaseInConditionalOnPropertyArchTest.java
Outdated
Show resolved
Hide resolved
Without this, the qa modules are not used during `./mvnw test` or `./mvnw verify`.
@mustafadagher I expected it would run in General / [UT] Run Unit Tests but it isn't because it's not run on
I'll try out the first option and see if that works. If you have another, let me know. |
@korthout in my other PR, that is based on your branch, I see them running and failing here, without a change to the root pom |
That workflow run checked out c73b402, which in turn has 3a39089 as a parent. So it already contained my fix. Note that PR workflows run against a merge between the PR head and base rather than on the PR head itself. |
The condition checks against uppercase characters only. This means it accepts kebab-case and not camelCase or PascalCase. Just staying not camelCase is thus not entirely correct. We can easily resolve this by removing the mention of camelCase here. Note that the validation already describes the actual violation (no uppercase allowed). I also took the chance to align both descriptions.
This change moves the RequireKebabCaseInConditionalOnPropertyArchTest to the io.camunda package as it covers all those classes. This makes it easier for other ArchTests to also align to the structure of the codebase that they work with. The end goal is a module that organizes its test in way where it's easy to see at a glance what code is covered by these tests.
7821ba6
to
42ce60f
Compare
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-36872-to-stable/8.5
git worktree add --checkout .worktree/backport-36872-to-stable/8.5 backport-36872-to-stable/8.5
cd .worktree/backport-36872-to-stable/8.5
git reset --hard HEAD^
git cherry-pick -x 43995116fc61966efa66a3acf1a68cb013b95ab4 d67a9f84a19353e7d3c5410cb6b309a8c1d446c5 d6f200e342211ccabb428950f0d13f5b292cf105 d8b567aace42680c722f73b9fcaffd10608eeefb feaf9335ea04da0a94bbb19ae0303c2295684cc6 b50ce94937798b416321ae169739a94f51267cf5 eba1343f4ce83b71e6d3043a0d029b11b86ab6ae 3a39089dd2e5b382b1f3b6d946411d74453ea02c 833b6b7e0abd1699089bc879fcb2d0bee7c5f3d8 42ce60f902b70c02de9c8d2d3eb4e0f66614bcc4
git push --force-with-lease |
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-36872-to-stable/operate-8.5
git worktree add --checkout .worktree/backport-36872-to-stable/operate-8.5 backport-36872-to-stable/operate-8.5
cd .worktree/backport-36872-to-stable/operate-8.5
git reset --hard HEAD^
git cherry-pick -x 43995116fc61966efa66a3acf1a68cb013b95ab4 d67a9f84a19353e7d3c5410cb6b309a8c1d446c5 d6f200e342211ccabb428950f0d13f5b292cf105 d8b567aace42680c722f73b9fcaffd10608eeefb feaf9335ea04da0a94bbb19ae0303c2295684cc6 b50ce94937798b416321ae169739a94f51267cf5 eba1343f4ce83b71e6d3043a0d029b11b86ab6ae 3a39089dd2e5b382b1f3b6d946411d74453ea02c 833b6b7e0abd1699089bc879fcb2d0bee7c5f3d8 42ce60f902b70c02de9c8d2d3eb4e0f66614bcc4
git push --force-with-lease |
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-36872-to-stable/8.6
git worktree add --checkout .worktree/backport-36872-to-stable/8.6 backport-36872-to-stable/8.6
cd .worktree/backport-36872-to-stable/8.6
git reset --hard HEAD^
git cherry-pick -x 43995116fc61966efa66a3acf1a68cb013b95ab4 d67a9f84a19353e7d3c5410cb6b309a8c1d446c5 d6f200e342211ccabb428950f0d13f5b292cf105 d8b567aace42680c722f73b9fcaffd10608eeefb feaf9335ea04da0a94bbb19ae0303c2295684cc6 b50ce94937798b416321ae169739a94f51267cf5 eba1343f4ce83b71e6d3043a0d029b11b86ab6ae 3a39089dd2e5b382b1f3b6d946411d74453ea02c 833b6b7e0abd1699089bc879fcb2d0bee7c5f3d8 42ce60f902b70c02de9c8d2d3eb4e0f66614bcc4
git push --force-with-lease |
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-36872-to-stable/8.7
git worktree add --checkout .worktree/backport-36872-to-stable/8.7 backport-36872-to-stable/8.7
cd .worktree/backport-36872-to-stable/8.7
git reset --hard HEAD^
git cherry-pick -x 43995116fc61966efa66a3acf1a68cb013b95ab4 d67a9f84a19353e7d3c5410cb6b309a8c1d446c5 d6f200e342211ccabb428950f0d13f5b292cf105 d8b567aace42680c722f73b9fcaffd10608eeefb feaf9335ea04da0a94bbb19ae0303c2295684cc6 b50ce94937798b416321ae169739a94f51267cf5 eba1343f4ce83b71e6d3043a0d029b11b86ab6ae 3a39089dd2e5b382b1f3b6d946411d74453ea02c 833b6b7e0abd1699089bc879fcb2d0bee7c5f3d8 42ce60f902b70c02de9c8d2d3eb4e0f66614bcc4
git push --force-with-lease |
@korthout I think this PR is causing the Maven release perform GHA to fail since it was merged: https://github.com/camunda/camunda/actions/runs/17143538365/job/48635366567 I assume that is because this PR adds |
Yes. Covered in:
Will re-add running them in CI with: |
Description
This PR introduces a new module
qa/archunit-test
(io.camunda:camunda-archunit-tests
) that can be used to define ArchUnit tests to cover the entire production code base.It also adds an ArchUnit test to this module
RequireKebabCaseInConditionalOnPropertyArchTest
which helps ensure that we support both camelCase and kebab-case for config properties. TheConditionalOnProperty
annotation requires the usage of kebab-case to support relaxed binding to properties.To satisfy the new rule, I've also updated several usages of
ConditionalOnProperty
towards the following properties:camunda.tasklist.webappEnabled
camunda.tasklist.webapp-enabled
camunda.operate.webappEnabled
camunda.operate.webapp-enabled
camunda.tasklist.importerEnabled
camunda.tasklist.importer-enabled
camunda.operate.importerEnabled
camunda.operate.importer-enabled
camunda.tasklist.featureFlag.processPublicEndpoints
camunda.tasklist.feature-flag.process-public-endpoints
This change is backward compatible.
Note
This PR does not yet move over existing ArchUnit tests to the new module. I'll raise a discussion about this separately through #37010.
Checklist
Related issues
closes #36869