-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add ability to enumerate monitoring team options in config #118
base: master
Are you sure you want to change the base?
Conversation
…umerated in a local file
dd6f772
to
6bd54ea
Compare
Pushed change to take team enumeration out of config, and instead record a path to a file where the teams are stored. |
admin/app/com/lucidchart/piezo/admin/controllers/TriggerFormHelper.scala
Outdated
Show resolved
Hide resolved
admin/app/com/lucidchart/piezo/admin/controllers/TriggerFormHelper.scala
Outdated
Show resolved
Hide resolved
import play.api.libs.json.JsArray | ||
|
||
class MonitoringTeams(configuration: Configuration) { | ||
private val path = configuration.getOptional[String]("com.lucidchart.piezo.admin.monitoringTeams.path") |
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 we have a separate config file for these teams instead of just including then directly in the configuration?
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.
After discussing with Hunter we decided we want to write a single source of truth for teams that can be accessible to both the admin server and our jobs server. Jobs will use it to map teams to slack channels. We could make chef write the teams both to piezo config and the reference file for our jobs server, but figured keeping it consistent made the most sense.
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.
FWIW, a hocon configuration file can import a json file. So it could just be something like:
include file("/etc/piezo/teams.json")
admin/app/com/lucidchart/piezo/admin/models/MonitoringTeams.scala
Outdated
Show resolved
Hide resolved
admin/public/js/triggerMonitoring.js
Outdated
@@ -0,0 +1,14 @@ | |||
(function () { |
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.
It isn't necessary to use an immediately invoked function evaluation like this in modern JS, you can accomplish the same thing with
(function () { | |
{ | |
const setMonitoringFieldVisibility = function() { | |
const priority = $('#triggerMonitoringPriority option:selected').val(); | |
if (priority === 'Off') { | |
$('#triggerMonitoringDetails').hide(); | |
} else { | |
$('#triggerMonitoringDetails').show(); | |
} | |
}; | |
$('#triggerMonitoringPriority').on('change', setMonitoringFieldVisibility); | |
setMonitoringFieldVisibility(); | |
} | |
Also, it is possible to avoid using jquery for this:
window.addEventListener('load', () => {
const priorityInput = document.getElementById('triggerMonitoringPriority');
const setMonitoringFieldVisibility = () => {
const priority = priorityInput.value;
const monitoringDetails = document.getElementById('triggerMonitoringDetails');
if (priority == 'Off') {
monitoringDetails.style.display = 'none'; // hide
} else {
monitoringDetails.style.display = 'block'; // show
}
};
priorityInput.addEventListener('change', setMonitoringFieldVisibility);
setMonitoringFieldVisibility();
}, {once: true});
Which would make it easier for us to eventually get rid of our jquery dependency.
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.
Nice call, I was wondering why the other js files were written this way.
I wasn't aware we wanted to move away from jquery here? Are there issues with using it?
admin/app/com/lucidchart/piezo/admin/models/MonitoringTeams.scala
Outdated
Show resolved
Hide resolved
class MonitoringTeams(configuration: Configuration) { | ||
private val path = configuration.getOptional[String]("com.lucidchart.piezo.admin.monitoringTeams.path") | ||
|
||
val value = path.flatMap(p => |
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.
val value = path.flatMap(p => | |
val teams = path.flatMap(p => |
?
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.
Originally had this but didn't love writing teams.teams
when referencing it, lol.
…eams enumerated in a local file
Addressed all feedback: 7104fb7 Ready for another review |
Optionally adds the ability to provide a list of teams in config. If provided, the freeform "Monitoring team" field on a trigger will change to a required dropdown. Monitoring fields will also be hidden when monitoring priority is Off.
This will allow us to enforce team ownership of job triggers (if monitoring is on).
A future change will set the list of teams on each jobs host in chef. Piezo's behavior is unchanged if there are no teams set in config, so this is fully backwards compatible.
If the team field has a value that is not in the list of valid teams once it's added, a valid team must be selected the next time the trigger is edited.