Skip to content

Fixes #27486: Add includeSystem parameter to filter system groups in API #6574

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.

Sign up for GitHub

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

Jump to bottom
Merged
clarktsiory merged 1 commit into from
Sep 9, 2025
Merged

Fixes #27486: Add includeSystem parameter to filter system groups in API #6574

clarktsiory merged 1 commit into from
Sep 9, 2025

Conversation

@clarktsiory
Copy link
Contributor

@clarktsiory clarktsiory commented Aug 26, 2025 •
edited
Loading

https://issues.rudder.io/issues/27486

Reusing the existing parameter extractor (used in the directives tree).
The filtering is also very similar, see

rudder/webapp/sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/lift/DirectiveApi.scala

Lines 280 to 288 in cd64fe5

def filterSystem(cat: FullActiveTechniqueCategory): FullActiveTechniqueCategory = {
cat.copy(
subCategories = cat.subCategories.filter(c => includeSystem || !c.isSystem).map(filterSystem).sortBy(_.name),
activeTechniques = cat.activeTechniques
.filter(c => includeSystem || c.policyTypes.isBase)
.map(t => t.copy(directives = t.directives.filterNot(_.isSystem).sortBy(_.name)))
.sortBy(_.techniqueName.value)
)
}

@clarktsiory clarktsiory requested a review from fanf August 26, 2025 15:27
fanf
fanf reviewed Aug 26, 2025
View reviewed changes
Copy link
Member

@fanf fanf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think an update in documentation is missing (it's a public api right?)

@clarktsiory
Copy link
Contributor Author

clarktsiory commented Aug 27, 2025 •
edited
Loading

PR updated with a new commit : public documentation update (also fixed directives tree documentation)

fanf
fanf reviewed Aug 28, 2025
View reviewed changes
webapp/sources/api-doc/paths/groups/tree.yml
required: false
schema:
type: boolean
default: true
Copy link
Member

@fanf fanf Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

humpf, so bad the default is not the same than in directive. Can we do an other PR for Rudder 9.0 that change the default value to "false" as it is in directiveTree for the API version corresponding to Rudder 9.0 (and keep the other default for previous API versions) ? If we don't do it now, we will forget and live with an inconsistancy.

The external effect is that user using that API won't see anymore system groups (ok), and we will need to correct where we call it in rules and groups at least I think.

Copy link
Contributor Author

@clarktsiory clarktsiory Aug 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK that's right, I took note to do that : https://issues.rudder.io/issues/27498

Copy link
Contributor Author

@clarktsiory clarktsiory Aug 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked it and changed in 9.0 with these PRs (it was only used in Rules, and system-updates) :

@clarktsiory clarktsiory requested a review from fanf September 4, 2025 08:24
@clarktsiory clarktsiory mentioned this pull request Sep 5, 2025
Merged
fanf
fanf approved these changes Sep 7, 2025
View reviewed changes
Copy link
Member

@fanf fanf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm now

@Normation-Quality-Assistant
Copy link
Contributor

Normation-Quality-Assistant commented Sep 9, 2025

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder/pull/6574
-- Your faithful QA
Kant merge: "Live your life as though your every act were to become a universal law."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/106916/console)

@clarktsiory
Copy link
Contributor Author

clarktsiory commented Sep 9, 2025

OK, squash merging this PR

@clarktsiory clarktsiory force-pushed the enh_27486/add_includesystem_parameter_to_filter_system_groups_in_api branch from cdc3c4e to 566222e Compare September 9, 2025 12:26
@clarktsiory clarktsiory merged commit 566222e into Normation:branches/rudder/8.3 Sep 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fanf fanf fanf approved these changes

Assignees

No one assigned

Labels

qa: Can't merge Ready for merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@clarktsiory @Normation-Quality-Assistant @fanf