Fixes #22338: Refactor some code and changes API url for compliance by directive #4674
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
amousset
reviewed
Feb 28, 2023
.../sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/ApiDatastructures.scala
Outdated
Show resolved
Hide resolved
ElaadF
commented
Feb 28, 2023
| val z = implicitly[Line].value | ||
| val description = "Get a directive's compliance to CSV format" | ||
| val (action, path) = GET / "compliance" / "directives" / "export" / "{id}" | ||
| val (action, path) = GET / "compliance" / "directives" / "{id}" |
There was a problem hiding this comment.
@VinceMacBuche you mentioned here that you will prefer this format: /compliance/directives/{id}?format=csv
I think it could be problematic because:
- the endpoint is the same as directive's compliance by ID, the two requests are the same for the webapp, since
?format=csvis not defined in the endpoint definition but we capture it at the moment we received the request - the archive API use the format
/archives/export?object=robject_id
There was a problem hiding this comment.
Yes i meant that csv and json should be the same endpoint, with a default format value to be json. I made a second commit with refactoring
|
Commit modified |
ElaadF
force-pushed
the
arch_22338/refactor_some_code_and_changes_api_url_for_compliance_by_directive
branch
from
69199ad
to
4664db2
Compare
ElaadF
added
the
WIP
|
PR updated with a new commit |
3 similar comments
|
PR updated with a new commit |
|
PR updated with a new commit |
|
PR updated with a new commit |
|
Commit modified |
VinceMacBuche
force-pushed
the
arch_22338/refactor_some_code_and_changes_api_url_for_compliance_by_directive
branch
2 times, most recently
from
2920bed
to
c36ee28
Compare
|
PR rebased |
ElaadF
force-pushed
the
arch_22338/refactor_some_code_and_changes_api_url_for_compliance_by_directive
branch
from
c36ee28
to
1bf6f1a
Compare
VinceMacBuche
force-pushed
the
arch_22338/refactor_some_code_and_changes_api_url_for_compliance_by_directive
branch
from
1bf6f1a
to
e986730
Compare
…y directive fixup! Fixes #22338: Refactor some code and changes API url for compliance by directive Fixes #22338: Refactor some code and changes API url for compliance by directive
ElaadF
force-pushed
the
arch_22338/refactor_some_code_and_changes_api_url_for_compliance_by_directive
branch
from
e986730
to
abe64ee
Compare
ElaadF
removed
the
WIP
VinceMacBuche
approved these changes
Mar 7, 2023
|
OK, merging this PR |
ncharles
reviewed
Mar 8, 2023
| precision.getOrElse(CompliancePrecision.Level2) | ||
| ) // by default, all details are displayed | ||
| val t4 = System.currentTimeMillis | ||
| TimingDebugLogger.trace(s"API DirectiveCompliance - serialize to json in ${t4 - t3} ms") |
There was a problem hiding this comment.
I have 5,5s on this part to show compliance per directive on load platform
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://issues.rudder.io/issues/22338