-
Notifications
You must be signed in to change notification settings - Fork 74
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 #22072: Display compliance by directive #4626
Fixes #22072: Display compliance by directive #4626
Conversation
3d32b54
to
af82377
Compare
Commit modified |
PR rebased |
af82377
to
61cedb0
Compare
PR updated with a new commit |
@@ -0,0 +1,40 @@ | |||
module FakeUtils exposing (..) |
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.
You should remove FakeUtils
PR updated with a new commit |
5 similar comments
PR updated with a new commit |
PR updated with a new commit |
PR updated with a new commit |
PR updated with a new commit |
PR updated with a new commit |
PR updated with a new commit |
1 similar comment
PR updated with a new commit |
PR rebased |
e6ce45c
to
0c690d9
Compare
PR rebased |
PR updated with a new commit |
PR rebased |
d99923f
to
c86a597
Compare
c86a597
to
5940526
Compare
PR updated with a new commit |
PR rebased |
da8f145
to
e4c151b
Compare
val dataContainer = Some("directivesCompliance") | ||
} | ||
|
||
final case object ExportDirectiveComplianceCSV extends ComplianceApi with OneParam with StartsAtVersion10 with SortIndex { |
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.
I would rather have the URL to be compliance/directives/id/csv
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.
(in fact i would prefer compliance/directives/id?format=csv
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.
Yes i would définitely prefer one endpoint and choose the format as a parameter
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.
why does it starts at version 10 and not the latest version ?
|
||
directive <- complianceService.getDirectiveCompliance(id, level) | ||
t3 = System.currentTimeMillis | ||
_ = TimingDebugLogger.trace(s"API Export compliance to CSV - getting query param in ${t2 - t1} ms") |
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.
Why do you put your traces at the end ? you shoud make them just below what they are doing
t4 = System.currentTimeMillis | ||
directives = directiveIds.flatMap(complianceService.getDirectiveCompliance(_, level)) | ||
t5 = System.currentTimeMillis | ||
_ = TimingDebugLogger.trace(s"API GetDirectives - getting query param in ${t2 - t1} ms") |
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.
same here and it misses some cases
precision.getOrElse(CompliancePrecision.Level2) | ||
) | ||
) // by default, all details are displayed | ||
val t4 = System.currentTimeMillis |
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.
Strange that it compiles with two T4
I'm merging but we should treat the feedbacks during beta 2 |
This PR is not mergeable to upper versions. |
OK, squash merging this PR |
Fixes #22072: Display compliance by directive Fixes #22072: Display compliance by directive fixup! fixup! Work in progress Fixes #22072: Display compliance by directive fixup! fixup! fixup! Work in progress Fixes #22072: Display compliance by directive fixup! fixup! fixup! fixup! Work in progress Fixes #22072: Display compliance by directive fixup! fixup! fixup! fixup! fixup! Work in progress Fixes #22072: Display compliance by directive fixup! fixup! fixup! fixup! fixup! fixup! Work in progress Fixes #22072: Display compliance by directive fixup! fixup! fixup! fixup! fixup! fixup! fixup! Work in progress Fixes #22072: Display compliance by directive fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! Work in progress Fixes #22072: Display compliance by directive fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! Work in progress Fixes #22072: Display compliance by directive fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! Work in progress Fixes #22072: Display compliance by directive fixup! fixup! Work in progress Fixes #22072: Display compliance by directive fixup! fixup! Work in progress Fixes #22072: Display compliance by directive
e4c151b
to
7c9df37
Compare
request | ||
{ method = "GET" | ||
, headers = [] | ||
, url = getUrl model ["nodes"] [] |
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.
i'm unsure as to why you need that. If it's really necessary, you could fetfh nly the minimal version of the node, or minimal+ exactly what you need
https://issues.rudder.io/issues/22072