Problem/Motivation

After sensor_info died, we have leftovers from previous naming sensor_info in drush and services.

We need to decide what / how these concepts are named to the outside... If we stick with sensor_info or change its name.

Proposed resolution

Decide about logic naming of the sensor_info / sensor instances exposed.

Change names, update the clients...

Remaining tasks

User interface changes

API changes

Comments

berdir’s picture

Shouldn't be easy to rename the plugin id and drush command to sensor-config? We could also go with just monitoring-sensor, but given that we have sensor-result as well...

Anushka-mp’s picture

Renamed quite some stuff. from sensor-info to sensor-config

Anushka-mp’s picture

Status: Active » Needs review
berdir’s picture

+++ b/monitoring.drush.inc
@@ -41,15 +41,15 @@ define('MONITORING_DRUSH_SENSOR_STATUS_OK', 0);
-  $items['monitoring-info'] = array(
+  $items['monitoring-config'] = array(
     'callback' => 'monitoring_drush_info',

Question here is if this makes sense I guess, or if it should be monitoring-sensor-config, but we just have monitoring-something for all commands and they are all sensor related.

miro_dietiker’s picture

Yeah, i think we should NOT drop "sensor" just because currently all commands are about sensors.

However, with the list of sensors, we need to decide if -config (previous -info) is wanted. Logically it's not about config only, it's a list of existing sensors.

berdir’s picture

Status: Needs review » Reviewed & tested by the community

There is no "sensor" to drop in the drush commands? I was wondering about the opposite, *adding* it. But I think that's a different discussion and we would have to rename everything then.

RTBC then I think. Note that this will change the public URL's, so we should make sure that's what we want and are going to keep it...

miro_dietiker’s picture

Status: Reviewed & tested by the community » Needs review

Hm, i think we should discuss this.
I thought about renaming the monitoring sensor list resource from monitoring-sensor-info to monitoring-sensor instead of monitoring-sensor-config.

If you look at the whole other refactoring Sensor => SensorPlugin and Sensor => SensorConfig, my suggestion was to stay with Sensor for everything that is not assigned to these two other concepts. The discussion is wether the list resource lists Sensors or Configurations. In my opinion, It should list Sensors (Instances of the SensorPlugin) and the fact that these are Config driven is an implementation detail from the Services perspective.

berdir’s picture

Status: Needs review » Needs work

As commented in #2384601: Rename Sensor to SensorPlugin, we should extract everything drush/services related from that patch that's not already here and do that separately.

Anushka-mp’s picture

Status: Needs work » Needs review
StatusFileSize
new3.79 KB
new10.18 KB

changed dush.inc as pointed out in #2384601: Rename Sensor to SensorPlugin.

Status: Needs review » Needs work

The last submitted patch, 9: monitoring-2383873-sensor-info-to-config-9.patch, failed testing.

berdir’s picture

  1. +++ b/modules/demo/monitoring_demo.install
    @@ -19,7 +19,7 @@ function monitoring_demo_install() {
    -    'monitoring-sensor-info' => array(
    +    'monitoring-sensor-config' => array(
    

    We decided to rename the rest plugin to just monitoring-sensor. Also remove the Config from the resource class name then.

    There are also some arguments in there, like expand on 'sensor_info', change that to 'sensor' as well.

    drush.inc and and both Resource classes should no longer contain any "sensor_info" or "sensor-info", probably also no "info" at all.

  2. +++ b/monitoring.drush.inc
    @@ -41,15 +41,15 @@ define('MONITORING_DRUSH_SENSOR_STATUS_OK', 0);
    -  $items['monitoring-info'] = array(
    +  $items['monitoring-sensor-config'] = array(
    ...
    -      'drush monitoring-info' => 'Prints sensor config for all available sensors.',
    -      'drush monitoring-info node_new' => 'Prints info of the node_new sensor.',
    +      'drush monitoring-config' => 'Prints sensor config for all available sensors.',
    +      'drush monitoring-config node_new' => 'Prints info of the node_new sensor.',
    

    Examples need to be updated.

Anushka-mp’s picture

Okay reworked, no more info or config.

Anushka-mp’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: monitoring-2383873-sensor-info-to-config-12.patch, failed testing.

Anushka-mp’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: monitoring-2383873-sensor-info-to-config-12.patch, failed testing.

miro_dietiker’s picture

Status: Needs work » Needs review
StatusFileSize
new19.03 KB

OK, added some further changes such as expand = sensor instead sensor_info.
Hope this passes now.

Status: Needs review » Needs work

The last submitted patch, 19: monitoring-2383873-sensor-config-18.patch, failed testing.

berdir’s picture

Yep, #2183983: Find hidden configuration schema issues happened, so all tests by default enforce a valid config schema.

Can be temporarily disabled by setting that flag in our base test classes to FALSE, I suggest we do that, and then work on the config schema issue, which now allows to easily validate that our config schema is correct.

Status: Needs work » Needs review
miro_dietiker’s picture

Committed the schema unchecking. Updated the issue. Let's see what the bots think about all this.

Status: Needs review » Needs work

The last submitted patch, 19: monitoring-2383873-sensor-config-18.patch, failed testing.

miro_dietiker’s picture

Status: Needs work » Needs review
StatusFileSize
new19.83 KB

Retrying :-)

miro_dietiker’s picture

Title: Decide about services sensor_info » Rename sensor_info: services to sensor / drush to sensor-config
Status: Needs review » Reviewed & tested by the community

Changing title...

miro_dietiker’s picture

Status: Reviewed & tested by the community » Fixed

Fixed this.

  • miro_dietiker committed 13b2eac on 8.x-1.x
    Issue #2383873 by Anushka-mp, miro_dietiker: Rename sensor_info:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.