Add support of environment variable for tab & group #3112

HiroyasuNishiyama · 2021-08-19T13:47:52Z

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Proposed changes

This draft PR adds support of environment variable definition feature to tabs and groups.
(design note: node-red/designs#56)

[test Flow]

[{"id":"f7ca862bdab6753c","type":"subflow","name":"Subflow 1","info":"","in":[{"x":40,"y":80,"wires":[{"id":"4252d927624bb8c7"}]}],"out":[{"x":360,"y":80,"wires":[{"id":"4252d927624bb8c7","port":0}]}]},{"id":"4252d927624bb8c7","type":"change","z":"f7ca862bdab6753c","name":"","rules":[{"t":"set","p":"payload","pt":"msg","to":"V0","tot":"env"}],"action":"","property":"","from":"","to":"","reg":false,"x":200,"y":80,"wires":[[]]},{"id":"24c1d7451d6b49db","type":"tab","label":"フロー 1","disabled":false,"info":"","env":[{"name":"V0","value":"tv0","type":"str"}]},{"id":"091be52d71e62cd2","type":"group","z":"24c1d7451d6b49db","name":"","style":{"label":true},"nodes":["0889f9fe85fcc1d8"],"env":[{"name":"V0","value":"out-v0","type":"str"}],"x":408,"y":533,"w":264,"h":134},{"id":"0889f9fe85fcc1d8","type":"group","z":"24c1d7451d6b49db","g":"091be52d71e62cd2","name":"","style":{"label":true},"nodes":["c7c7d54f923f9259"],"env":[{"name":"V0","value":"in-v0","type":"str"}],"x":434,"y":559,"w":212,"h":82},{"id":"5ffeaf880234c887","type":"group","z":"24c1d7451d6b49db","name":"","style":{"label":true},"nodes":["d0568e513ac905b2"],"env":[{"name":"V0","value":"gv0","type":"str"}],"x":454,"y":839,"w":172,"h":82},{"id":"601d768471c7d05c","type":"group","z":"24c1d7451d6b49db","name":"","style":{"label":true},"nodes":["ad41555e1315ced4"],"env":[{"name":"V0","value":"gv0","type":"str"}],"x":434,"y":399,"w":212,"h":82},{"id":"8401fc7b99c14ee7","type":"group","z":"24c1d7451d6b49db","name":"","style":{"label":true},"nodes":["f11f724f5afa502a"],"env":[{"name":"V0","value":"gv0","type":"str"}],"x":454,"y":719,"w":172,"h":82},{"id":"44a1c7f8c1b8d1e6","type":"inject","z":"24c1d7451d6b49db","name":"","props":[{"p":"payload"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","payload":"","payloadType":"date","x":300,"y":300,"wires":[["956486923054767c"]]},{"id":"e06f5fdc6d9fdc8f","type":"debug","z":"24c1d7451d6b49db","name":"","active":true,"tosidebar":true,"console":false,"tostatus":false,"complete":"false","statusVal":"","statusType":"auto","x":770,"y":300,"wires":[]},{"id":"956486923054767c","type":"change","z":"24c1d7451d6b49db","name":"","rules":[{"t":"set","p":"payload","pt":"msg","to":"V0","tot":"env"}],"action":"","property":"","from":"","to":"","reg":false,"x":540,"y":300,"wires":[["e06f5fdc6d9fdc8f"]]},{"id":"b18f175017564f48","type":"inject","z":"24c1d7451d6b49db","name":"","props":[{"p":"payload"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","payload":"","payloadType":"date","x":300,"y":440,"wires":[["ad41555e1315ced4"]]},{"id":"084382515d716b6b","type":"debug","z":"24c1d7451d6b49db","name":"","active":true,"tosidebar":true,"console":false,"tostatus":false,"complete":"false","statusVal":"","statusType":"auto","x":770,"y":440,"wires":[]},{"id":"ad41555e1315ced4","type":"change","z":"24c1d7451d6b49db","g":"601d768471c7d05c","name":"","rules":[{"t":"set","p":"payload","pt":"msg","to":"V0","tot":"env"}],"action":"","property":"","from":"","to":"","reg":false,"x":540,"y":440,"wires":[["084382515d716b6b"]]},{"id":"24c56aecce820e28","type":"inject","z":"24c1d7451d6b49db","name":"","props":[{"p":"payload"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","payload":"","payloadType":"date","x":300,"y":600,"wires":[["c7c7d54f923f9259"]]},{"id":"ee126dae99dd3844","type":"debug","z":"24c1d7451d6b49db","name":"","active":true,"tosidebar":true,"console":false,"tostatus":false,"complete":"false","statusVal":"","statusType":"auto","x":770,"y":600,"wires":[]},{"id":"c7c7d54f923f9259","type":"change","z":"24c1d7451d6b49db","g":"0889f9fe85fcc1d8","name":"","rules":[{"t":"set","p":"payload","pt":"msg","to":"$parent.V0","tot":"env"}],"action":"","property":"","from":"","to":"","reg":false,"x":540,"y":600,"wires":[["ee126dae99dd3844"]]},{"id":"15c7101fd32da0e7","type":"inject","z":"24c1d7451d6b49db","name":"","props":[{"p":"payload"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","payload":"","payloadType":"date","x":300,"y":760,"wires":[["f11f724f5afa502a"]]},{"id":"36576de7a7a74e66","type":"debug","z":"24c1d7451d6b49db","name":"","active":true,"tosidebar":true,"console":false,"tostatus":false,"complete":"false","statusVal":"","statusType":"auto","x":770,"y":760,"wires":[]},{"id":"f11f724f5afa502a","type":"subflow:f7ca862bdab6753c","z":"24c1d7451d6b49db","g":"8401fc7b99c14ee7","x":540,"y":760,"wires":[["36576de7a7a74e66"]]},{"id":"f53112ebe646638d","type":"inject","z":"24c1d7451d6b49db","name":"","props":[{"p":"payload"},{"p":"topic","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","payload":"","payloadType":"date","x":300,"y":880,"wires":[["d0568e513ac905b2"]]},{"id":"a0ed130a81a70a9e","type":"debug","z":"24c1d7451d6b49db","name":"","active":true,"tosidebar":true,"console":false,"tostatus":false,"complete":"false","statusVal":"","statusType":"auto","x":770,"y":880,"wires":[]},{"id":"d0568e513ac905b2","type":"subflow:f7ca862bdab6753c","z":"24c1d7451d6b49db","g":"5ffeaf880234c887","name":"","env":[{"name":"V0","value":"sv0","type":"str"}],"x":540,"y":880,"wires":[["a0ed130a81a70a9e"]]}]

Checklist

I have read the contribution guidelines
For non-bugfix PRs, I have discussed this change on the forum/slack team.
I have run grunt to verify the unit tests pass
I have added suitable unit tests to cover the new/changed functionality

coveralls · 2021-08-19T13:51:53Z

Coverage decreased (-0.009%) to 67.684% when pulling 67404a3 on node-red-hitachi:group-tab-env-var into f1e7ec0 on node-red:master.

knolleary

A few comments for your feedback.

packages/node_modules/@node-red/runtime/lib/flows/Subflow.js

packages/node_modules/@node-red/runtime/lib/flows/Flow.js

packages/node_modules/@node-red/editor-client/src/js/ui/workspaces.js

knolleary

At the moment, I'm not sure the approach to getting a Group setting is right - the logic is outside of Flow.js. It feels to me like Flow should own that logic.

At the moment we have getSetting(name) on the Flow object. That gets you the setting for the flow (or returns the process.env value).

From an API design, I think it would be cleaner to have something like getNodeSetting(node,name) that does the work to check if the node is in a group, handle nested groups, and then fallback to getSetting(name).

But that is a piece of refactoring I'm happy to make later, if we're happy with the overall functionality.

knolleary · 2021-08-31T15:30:16Z

packages/node_modules/@node-red/runtime/lib/flows/Flow.js

+     * @param  {String} key
+     * @return {Object} result containinig val property or null
+     */
+    getFlowSetting(name) {


We already have getSetting defined on the Flow object. Why have you separated this out? I can see getSetting now calls this function - but I cannot see it being called from anywhere else.

Merged them, as there is no particular reason for them to be separated.

knolleary · 2021-09-01T19:11:58Z

Can you retarget this for the dev branch? Otherwise I think it's okay to merge.

As part of the Flow Testing requirement to be able to add custom edit panes to the dialogs, I've been working on a refactoring of editor.js to make each 'tab' inside the edit dialog a self-contained component. That is going to impact this feature - as the env list can become a reusable edit pane and we should be able to reduce the amount of duplicate code. But I'll take care of that once this is merged.

HiroyasuNishiyama · 2021-09-02T00:58:24Z

Retargeted to dev branch.

knolleary · 2021-09-02T09:22:46Z

I assume you are happy for me to merge this PR? It is still in draft state.

HiroyasuNishiyama · 2021-09-02T11:32:51Z

Yes. Of course. I am happy that you will merge this PR.

knolleary · 2021-09-07T12:46:49Z

Merged as part of #3130

add support of environtment variable for tab & group

6aecc39

knolleary requested changes Aug 23, 2021

View reviewed changes

update according to PR comments

d78e593

knolleary reviewed Aug 31, 2021

View reviewed changes

merge getFlowSetting to getSetting

67404a3

HiroyasuNishiyama changed the base branch from master to dev September 2, 2021 00:55

HiroyasuNishiyama marked this pull request as ready for review September 2, 2021 11:32

knolleary mentioned this pull request Sep 2, 2021

Refactor editor dialog code to increase code reuse between edit panes #3130

Merged

knolleary merged commit 67404a3 into node-red:dev Sep 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support of environment variable for tab & group #3112

Add support of environment variable for tab & group #3112

HiroyasuNishiyama commented Aug 19, 2021

coveralls commented Aug 19, 2021 •

edited

knolleary left a comment

knolleary left a comment

knolleary Aug 31, 2021

HiroyasuNishiyama Sep 1, 2021

knolleary commented Sep 1, 2021

HiroyasuNishiyama commented Sep 2, 2021

knolleary commented Sep 2, 2021

HiroyasuNishiyama commented Sep 2, 2021

knolleary commented Sep 7, 2021

Add support of environment variable for tab & group #3112

Add support of environment variable for tab & group #3112

Conversation

HiroyasuNishiyama commented Aug 19, 2021

Proposed changes

Checklist

coveralls commented Aug 19, 2021 • edited

knolleary left a comment

Choose a reason for hiding this comment

knolleary left a comment

Choose a reason for hiding this comment

knolleary Aug 31, 2021

Choose a reason for hiding this comment

HiroyasuNishiyama Sep 1, 2021

Choose a reason for hiding this comment

knolleary commented Sep 1, 2021

HiroyasuNishiyama commented Sep 2, 2021

knolleary commented Sep 2, 2021

HiroyasuNishiyama commented Sep 2, 2021

knolleary commented Sep 7, 2021

coveralls commented Aug 19, 2021 •

edited