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 #15531: Add an Interface to select reporting protocol #2411
Fixes #15531: Add an Interface to select reporting protocol #2411
Conversation
@@ -174,26 +174,52 @@ <h3 class="page-title">Security</h3> | |||
</form> | |||
</div> | |||
</div> | |||
<div class="inner-portlet"> | |||
<h3 class="page-title">Reporting protocol</h3> | |||
<div class="lift:administration.PropertiesManagement.reportProtocolSection" id="reportProtocolForm"> |
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.
there need an introduction text about the protocol, that Syslog is the historical one, but in 5.1 HTTPS is available, better, faster, stronger, safer. Or something like that
5da894b
to
f77a673
Compare
Commit modified |
f77a673
to
82ddd3a
Compare
Commit modified |
82ddd3a
to
32831b1
Compare
Commit modified |
32831b1
to
f31de82
Compare
Commit modified |
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.
some minor cosmetic changes plus a question on a input
|
||
def noModif = ( | ||
for { | ||
initSyslog <- initSyslogProtocol |
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.
could you correct the indentation ?
configService.set_rudder_syslog_protocol_disabled(disabledSyslog, actor, None).toBox.foreach(updateOk => initDisabledSyslog = Full(disabledSyslog)) | ||
|
||
// start a promise generation, Since we check if there is change to save, if we got there it mean that we need to redeploy | ||
startNewPolicyGeneration |
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.
the configService should take care of starting the new policy generation if value changed (at least I think that's how it works)
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.
need to look at the service!
reportProtocol = protocol | ||
protocol match { | ||
case AgentReportingHTTPS => | ||
|
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.
could you remove this blank line which is misleading ?
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.
and the one below
|
||
val checkboxInitValue = initReportsProtocol == SyslogUDP | ||
val zip : Box[(AgentReportingProtocol,Boolean)] = (initReportProtocol,initDisabledSyslog) match { |
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.
zip ??
val (label,value) = labelAndValue(protocol,disabledSyslog) | ||
val inputId = value + "-id" | ||
val ajaxCall = SHtml.ajaxCall(Str(""), _ => displayDisableSyslogSectionJS(value) & check)._2.toJsCmd | ||
val inputCheck = if (initReport == protocol && initDisabled == disabledSyslog) { |
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 the condition on disabledSyslog ?
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.
(or on protocol - i don't get why there is both)
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.
Because we manage both properties with that input ... it' a three state select
With
Https only (so https + disabled syslog)
https with syslog support (so https + enable syslog)
Syslog only (so Syslog and enable syslog (obviously)
f31de82
to
29c6891
Compare
Commit modified |
OK, merging this PR |
https://issues.rudder.io/issues/15531