add no_hardware to pubber options #340

noursaidi · 2022-05-25T09:46:26Z

Add pubber runtime option to omit hardware field to trigger validation error (missing required field)

grafnu

I'm not convinced this is the right way for us to add lots of configuration options to pubber. I'd like to understand the motivating use case a bit more. Ideally, more of this would be encapsulated in the metadata setup rather than as line-items in a script. But, that would explicitly depend on the use-cases that are being explored. Basically, this won't scale well, so we should have an understanding of the potential complexity (how many options?) this is intended to serve. Maybe we can briefly talk about it in our meeting today.

noursaidi · 2022-05-26T10:12:29Z

@grafnu PTAL - the options are now an object in the configuration file

grafnu · 2022-05-26T15:24:16Z

bin/pubber

        else
-            options[$option]=$option
+            k=$option
+            v=$option


I'm wondering if this should be a boolean. Ideally we can be more semantic about this, and having it as a boolean would allow us to make sure people are using options correctly...

Agree, turned it into a boolean with correct type assignment for checking

grafnu · 2022-05-26T15:26:38Z

pubber/src/main/java/daq/pubber/Configuration.java

  public String serialNo;
  public String macAddr;
-  public String extraPoint;
+  public HashMap<String, String> options = new HashMap<>();


I would make this an explicit Options object. The reason is once you go to a general map here you lose the ability to have any checking on the contents and types. Ideally, this structure would be specified as a protobuf or json schema itself, so we get the auto-documenting process with that. It also makes the code simpler since it's not a lot of key-lookups-in-maps and makes the code itself more stable through any changes.

Turned options into an options object

…rhardware

noursaidi · 2022-06-07T11:41:13Z

@grafnu PTAL if the changes now address the comments

grafnu · 2022-06-07T14:07:04Z

bin/pubber

@@ -31,29 +31,26 @@ else
        if [[ $option == *"="* ]]; then
            k=$(echo $option | cut -d'=' -f1)
            v=$(echo $option | cut -d'=' -f2)
-            options[$k]=$v
+            if ! [[ $v =~ ^[0-9]+([.][0-9]+)?$ ]]; then


I wouldn't go about it this way, since although it seems "simple" it's actually somewhat of a minefield because, say, what if you want a string that looks like a number? Better to be just simpler (all options are strings) and then deal with the conversion later, if/when necessary.

Removed so options are either boolean true or string value

grafnu · 2022-06-07T14:08:47Z

pubber/src/main/java/daq/pubber/Pubber.java

-      addPoint(makePoint(configuration.extraPoint,
+    // Pubber runtime options
+    if (configuration.options.extraField) {
+      devicePoints.extraField = "extra_field";


see, now if this were still a string then you have your extra field value!

grafnu · 2022-06-07T14:08:59Z

pubber/src/main/java/daq/pubber/Pubber.java

+    }
+
+    if (configuration.options.extraPoint) {
+      addPoint(makePoint("extra_point",


ditto -- extra_point here would be useful

extra point value now taken from string

grafnu · 2022-06-07T14:10:19Z

pubber/src/main/java/daq/pubber/Pubber.java

-      addPoint(makePoint(configuration.extraPoint,
+    // Pubber runtime options
+    if (configuration.options.extraField) {
+      devicePoints.extraField = "extra_field";


see, now if this were still a string then you have your extra field value!

extra field value now taken from string

bin/pubber

grafnu · 2022-06-07T14:16:48Z

bin/pubber

-        printf -v extra_field '"extraField": "%s",' "${options[extra_field]}"
-    fi
-
+    options_json="{${options_json%?}}"


what's the ? for? If it's a wildcard, can you be explicit about what it's removing (trailing comma, right?)

last character, changed to remove trailing comma

grafnu · 2022-06-07T14:17:54Z

pubber/src/main/java/daq/pubber/Pubber.java

        configuration.gatewayId));

    deviceState.system.operational = true;
    deviceState.system.serial_no = configuration.serialNo;
    deviceState.system.hardware.make = "BOS";
    deviceState.system.hardware.model = "pubber";
    deviceState.system.software = new HashMap<>();
-    deviceState.system.software.put("firmware", "v1");
+    deviceState.system.software.put("firmware", "v1");    


grafnu · 2022-06-09T13:12:22Z

proxy/pubber.json

-  "deviceId": "SNS-4"
+  "deviceId": "SNS-4",
+  "options": {
+    "extraField": true


shouldn't extraField be a string, not boolean?

grafnu · 2022-06-09T13:12:58Z

pubber/src/main/java/daq/pubber/ConfigurationOptions.java

+ */
+public class ConfigurationOptions {
+  public Boolean noHardware = false;
+  public String extraPoint = "";


don't assign an empty string to undefined values -- there's a semantic difference between them -- so these should all default to null (no assignment).

@grafnu thanks! should that extend to noHardware too which I've assigned a default value? Then the check is != null && !configuration.optionsnoHardware?

grafnu · 2022-06-09T13:13:54Z

pubber/src/main/java/daq/pubber/Pubber.java

-    if (configuration.extraPoint != null && !configuration.extraPoint.isEmpty()) {
-      addPoint(makePoint(configuration.extraPoint,
+    // Pubber runtime options
+    if (!configuration.options.extraField.isEmpty()) {


should be "extraField != null" -- if somebody really wants to specify an empty extra field then they should be able to.

grafnu · 2022-06-09T13:14:27Z

pubber/src/main/java/daq/pubber/Pubber.java

+      devicePoints.extraField = configuration.options.extraField;
+    }
+
+    if (!configuration.options.extraPoint.isEmpty()) {


semantically this should be != null, and then something else would throw an error saying "can't define an empty point name" (or just ignore that case because it's all for error checking anyway)

grafnu · 2022-06-09T20:02:34Z

Correct... generally avoid a default unless it's really really useful/necessary.

…

On Thu, Jun 9, 2022 at 12:06 PM Noureddine ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pubber/src/main/java/daq/pubber/ConfigurationOptions.java <#340 (comment)>: > @@ -0,0 +1,11 @@ +package daq.pubber; + +/** + * Pubber configuration options which change default behavior. + */ +public class ConfigurationOptions { + public Boolean noHardware = false; + public String extraPoint = ""; @grafnu <https://github.com/grafnu> thanks! should that extend to noHardware too which I've assigned a default value? Then the check is != null && !configuration.optionsnoHardware? — Reply to this email directly, view it on GitHub <#340 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIEPD3VME6N3TLDL3OOSXLVOI6EXANCNFSM5W4QTQJA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

grafnu

Brilliant. Thanks for working through all the iterations with me!

noursaidi added 2 commits May 25, 2022 10:41

add no_hardware to pubber opts

c248416

docs

0ab1555

noursaidi requested a review from grafnu May 25, 2022 09:46

grafnu reviewed May 25, 2022

View reviewed changes

dynamic options object

ebaf1a7

noursaidi force-pushed the pubberhardware branch from 585a862 to ebaf1a7 Compare May 26, 2022 10:09

noursaidi requested a review from grafnu May 26, 2022 10:12

update pubber.json

b771921

grafnu reviewed May 26, 2022

View reviewed changes

use options object

929e285

noursaidi force-pushed the pubberhardware branch from 85bf1db to 929e285 Compare June 7, 2022 09:43

noursaidi added 2 commits June 7, 2022 10:48

pubber assignable options

fc3bb3f

handle and enforce types

d16175b

noursaidi force-pushed the pubberhardware branch from f6ce069 to d16175b Compare June 7, 2022 10:26

noursaidi added 6 commits June 7, 2022 11:30

fix noHardware logic

d91f9bb

fix noHardware logic

6cb12c6

Merge branch 'master' into pubberhardware

8b88953

fix style errors and remove tostring

0da054b

Merge branch 'pubberhardware' of github.com:noursaidi/udmi into pubbe…

de43900

…rhardware

fix style

691911a

noursaidi requested a review from grafnu June 7, 2022 11:40

remove unused imports

900d940

grafnu reviewed Jun 7, 2022

View reviewed changes

comments .. type and explicit string manip

237ec8a

noursaidi force-pushed the pubberhardware branch from 5e65ea5 to 237ec8a Compare June 9, 2022 08:44

remove extra white space

a882177

noursaidi requested a review from grafnu June 9, 2022 08:52

noursaidi mentioned this pull request Jun 9, 2022

Add pubber options - missing point and config ACK (QoS 0) #359

Closed

grafnu reviewed Jun 9, 2022

View reviewed changes

no empty check

ef8fbfa

comments + docs

7df2aa0

noursaidi force-pushed the pubberhardware branch from 97afa88 to 7df2aa0 Compare June 10, 2022 10:41

correct documentation

d0aded8

noursaidi requested a review from grafnu June 10, 2022 11:43

grafnu approved these changes Jun 10, 2022

View reviewed changes

noursaidi merged commit e215679 into faucetsdn:master Jun 13, 2022

noursaidi deleted the pubberhardware branch September 20, 2022 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add no_hardware to pubber options #340

add no_hardware to pubber options #340

noursaidi commented May 25, 2022

grafnu left a comment

noursaidi commented May 26, 2022

grafnu May 26, 2022

noursaidi Jun 7, 2022

grafnu May 26, 2022

noursaidi Jun 7, 2022

noursaidi commented Jun 7, 2022

grafnu Jun 7, 2022

noursaidi Jun 9, 2022

grafnu Jun 7, 2022

grafnu Jun 7, 2022

noursaidi Jun 9, 2022

grafnu Jun 7, 2022

noursaidi Jun 9, 2022

grafnu Jun 7, 2022

noursaidi Jun 9, 2022

grafnu Jun 7, 2022

grafnu Jun 9, 2022

grafnu Jun 9, 2022

noursaidi Jun 9, 2022

grafnu Jun 9, 2022

grafnu Jun 9, 2022

grafnu commented Jun 9, 2022 via email

grafnu left a comment

add no_hardware to pubber options #340

add no_hardware to pubber options #340

Conversation

noursaidi commented May 25, 2022

grafnu left a comment

Choose a reason for hiding this comment

noursaidi commented May 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noursaidi commented Jun 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grafnu commented Jun 9, 2022 via email

grafnu left a comment

Choose a reason for hiding this comment