Skip to content

Conversation

@bimalyn-IBM
Copy link
Contributor

Removed default value and required attribute from the community field in the snmp node.

This was preventing a user from being able to set the community value within the msg.

This issue was noticed in #311.

… can be defined by the msg.

Error will be thrown if you try to override what was defined in the node.
Verified that the contents of msg is no longer clobbered.

Signed-off-by: Bryan Malyn <bimalyn@us.ibm.com>
Signed-off-by: Bryan Malyn <bimalyn@us.ibm.com>
Signed-off-by: Bryan Malyn <bimalyn@us.ibm.com>
Signed-off-by: Bryan Malyn <bimalyn@us.ibm.com>
Signed-off-by: Bryan Malyn <bimalyn@us.ibm.com>
fixed documentation as noted, removed conflict and iff from documention
remove node.warns if host or community are set in both the node config and msg

Signed-off-by: Bryan Malyn <bimalyn@us.ibm.com>
Signed-off-by: Bryan Malyn <bimalyn@us.ibm.com>
This commit uses a singleton aproach to create socket.
It still needs to be tested to see if there is any issue to the never-close,
but reuse socket model. My only concern is if a socket dies, do we need to do something to reestablish it?

Signed-off-by: Bryan Malyn <bimalyn@us.ibm.com>
attribute in the snmp node.

This was preventing a user from being able to set the community
value within the msg.

Signed-off-by: Bryan Malyn <bimalyn@us.ibm.com>
@dceejay
Copy link
Member

dceejay commented May 31, 2017

so now it won't work by default ? - should leave the default value... - users should remove it if they then want to override.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 76.744% when pulling b1da7fc on bimalyn-IBM:master into d4d5b1f on node-red:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 76.744% when pulling b1da7fc on bimalyn-IBM:master into d4d5b1f on node-red:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 76.744% when pulling b1da7fc on bimalyn-IBM:master into d4d5b1f on node-red:master.

@bimalyn-IBM
Copy link
Contributor Author

bimalyn-IBM commented May 31, 2017

It should still work by default. We set

var community = node.community || msg.community;

which gets thrown into

snmp.createSession(host, community, {version: version});

which creates a Session (see here for full context):

var Session = function (target, community, options) {
	this.target = target || "127.0.0.1";
        this.community = community || "public";
...

If we want to, for clarity's sake, still have public be the default value of community, we should probably also make 127.0.0.1 or localhost be the default value for host too.

@dceejay
Copy link
Member

dceejay commented May 31, 2017

Agree with your last comment. I like clarity.

…them

to be set so that they can be set my msg.host and msg.community respectivly.

Signed-off-by: Bryan Malyn <bimalyn@us.ibm.com>
@coveralls
Copy link

coveralls commented May 31, 2017

Coverage Status

Coverage remained the same at 76.744% when pulling ea9949a on bimalyn-IBM:master into d4d5b1f on node-red:master.

@dceejay dceejay merged commit 78a6af1 into node-red:master May 31, 2017
@bimalyn-IBM bimalyn-IBM mentioned this pull request Jun 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants