Skip to content
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

Fixes #24363: Display compliance for system groups #5449

Conversation

clarktsiory
Copy link
Contributor

@clarktsiory clarktsiory commented Mar 7, 2024

https://issues.rudder.io/issues/24363

We need to support either a groupId (e.g. an uuid) or a target (e.g. special:... or even group:...).

Common fields that we need for the compliance result are id, name, serverList, the code is refactored accordingly.

Screenshots :

Screenshot from 2024-03-08 14-37-36

Screenshot from 2024-03-08 14-34-00

This one needs some change in the UI (in another PR) :
image

@clarktsiory clarktsiory marked this pull request as draft March 7, 2024 11:54
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

@clarktsiory clarktsiory marked this pull request as ready for review March 8, 2024 13:46
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

@@ -838,7 +837,7 @@ object JsonCompliance {
implicit class JsonByNodeGroupCompliance(val nodeGroup: ByNodeGroupCompliance) extends AnyVal {

def toJson(level: Int, precision: CompliancePrecision): JObject = {
(("id" -> nodeGroup.id.serialize)
Copy link
Member

Choose a reason for hiding this comment

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

why not using the serialisation of node group as a SimpleTarget ? That datastructure is here just for that (ie abstracting over the different kind of targets: groups, system ones, etc).
You will get a string lile: group:90994f11-1638-4945-add0-70dc543e80d0 or special:all or policyServer:root etc.

And then just always parse back a SimpleTarget, and fail - ie parseSimpleTargetOrNodeGroupId does not seems necessary and inconsistent with the general way of representing rule targets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is indeed clearer to directly use it within the API.
As a consequence, the Elm app will need to reason over both target and group ids, because it parses and uses the raw group uuid as returned in the group tree. The change is here : 5c521e4

And I am not sure if parseSimpleTargetOrNodeGroupId should actually be removed, we have an API endpoint that looks like api/compliance/groups/9362944f-781d-4d9c-b8af-bfcdee8e63bb now, and we should assume this is the basic "Public API" interface, but also allow for the api/compliance/groups/group:9362944f-781d-4d9c-b8af-bfcdee8e63bb.
I don't think we want to return "not found" with : api/compliance/groups/9362944f-781d-4d9c-b8af-bfcdee8e63bb.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes. It's a public API used internally.
OK, if it's not only a private API, api/compliance/groups/group:9362944f-781d-4d9c-b8af-bfcdee8e63bb doesn't make much sense for an user, esp since we don't have that anywhere in the public API (plus the fact it's "group" and not "target" in the url).

Could it be api/compliance/targets/group:... - it would at least bring some consistency. Perhaps we need to ask other.

Copy link
Member

Choose a reason for hiding this comment

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

I do think it should accept both values: a node group id directly and a target (including group:nodeid) and it should be an either Here. or above you can translate something that does not match a target to a groupTarget

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed that's exactly what the parse function does and always ends up with a target :

private[this] def parseSimpleTargetOrNodeGroupId(str: String): PureResult[SimpleTarget] = {
    // attempt to parse a "target" first because format is more specific
    RuleTarget.unserOne(str) match {
      case None        => NodeGroupId.parse(str).map(GroupTarget(_)).left.map(Inconsistency(_))
      case Some(value) => Right(value)
    }
  }

Copy link
Member

Choose a reason for hiding this comment

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

I think you should answer with the id and not the target in json response

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I should just revert the whole commit that replaced everything with a target : 5c521e4 ?

Copy link
Member

Choose a reason for hiding this comment

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

OK, so this kind of subject gives more whatever to the idea that elm application should only use internal, RPC like APIs, and that public API have a different set of goals and contracts.

In that example, the correct, simple RPC like thing that the elm app need is in contradiction with what user expects : elm does care about writting .../group:9362944f-781d-4d9c-b8af-bfcdee8e63b, but it's very weird for an human.

Since that API is a public one, yes we need to support the scheme and convention we use elsewhere, so:

  • .../groups/9362944f-781d-4d9c-b8af-bfcdee8e63b needs to be supported for human convenience in the scala API,
  • .../groups/special:all needs to be supported because the model is like that.

And so, even if the elm app always use the correct, full serialisation of target, the backend needs to have a parser like the one you had initially.

Sorry to have missed the fact that the API was public and set you on a bad path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, it is not an issue, I didn't emphasis on the API being public either...

So I am reverting the change, and for the details, Elm also doesn't always make requests with group:... but directly with the UUID because it receives the UUID from the "group tree"

@fanf
Copy link
Member

fanf commented Mar 12, 2024

There is a CSS problem when there is a lot of nodes:
image

Please open an other ticket to correct it indepently from that one (ie it doesn't already exist).

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.

Appart for the serialization subject, it seems to work as expected. Once corrected, you will be able to just add the "ready for merge" tag. Plus need to open an other ticket for the css problem when there is a lot of nodes.

@clarktsiory
Copy link
Contributor Author

PR rebased

@clarktsiory clarktsiory force-pushed the bug_24363/display_compliance_for_system_groups branch from ffd5a29 to ee3b801 Compare March 12, 2024 11:55
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

@clarktsiory clarktsiory force-pushed the bug_24363/display_compliance_for_system_groups branch from 5c521e4 to ee3b801 Compare March 13, 2024 10:03
@Normation-Quality-Assistant
Copy link
Contributor

OK, merging this PR

@Normation-Quality-Assistant Normation-Quality-Assistant merged commit 06d64cc into Normation:branches/rudder/8.1 Mar 13, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants