-
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 #8534: Add an technique API endpoint to display directives based on a technique #1117
Conversation
directive <- directives | ||
if ( versions match { | ||
case None => true | ||
case Some(versions) => versions.contains(directive.techniqueVersion) |
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.
would be clearer to not override versions variable name
<3 this will make life easier. |
Commit modified |
41079a3
to
51458d4
Compare
Commit modified |
51458d4
to
43aa519
Compare
The two endpoints works as advertised, but I find it very hard to use them without an "index" endpoint in /techniques/, which would just output techniques -> [versions] for example. Would it be hard to add ? |
Well, or you need to have a rudder UI open, which is a little sad for an API ;( |
(oups) |
Commit modified |
43aa519
to
536aec0
Compare
} | ||
|
||
def listDirectives (techniqueName : TechniqueName, wantedVersions: Option[List[TechniqueVersion]]) : Box[JValue] = { | ||
def serializeDirectives (directives : Seq[Directive], techniques : SortedMap[TechniqueVersion, Technique]) = { |
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 don't like having closed parameters used in subfunction. It's harder to refactor, and closure leaks all around in scala. Could you add the wantedVersions parameter to parameters of serializeDirectives?
Appart for the cosmetic remarks, it's ok: you can mark it as mergeable when ready! |
Commit modified |
536aec0
to
02f0dca
Compare
This PR is not mergeable to upper versions. |
OK, merging this PR |
https://www.rudder-project.org/redmine/issues/8534