Skip to content
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

Fix bug #113 #119

Merged
merged 16 commits into from
Mar 19, 2018
Merged

Fix bug #113 #119

merged 16 commits into from
Mar 19, 2018

Conversation

dcalvoalonso
Copy link
Contributor

  • Correct configuration API example
  • Correct management of active attributes in configurations

@@ -57,7 +57,7 @@ function mapConfig(device, configuration, callback) {
service: configuration.service,
subservice: configuration.subservice,
lazy: configuration.lazy,
attributes: configuration.attributes,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related with telefonicaid/iotagent-node-lib#588...

I understand this is an internal variable not related with the keyword used in payload (i.e. "attributes"). Is my interpretation correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, this is the field used in the payload to describe attributes. So this was an error caused by telefonicaid/iotagent-node-lib#588

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, this is the field used in the payload to describe attributes

You mean to configuration.attributes, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's right

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NTC

@@ -81,7 +81,7 @@ Lightweight M2M port.
The following request creates the configuration group for devices with type `WeatherBaloon`:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is related with PR #118? In that case I understand no new entry in CNR is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. NTC

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...although, on the other side, PR is mentioning #113. Should an entry for #113 be added to CNR file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right! Someday I will not forget the CNR!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c9820c8

fgalan
fgalan previously approved these changes Mar 19, 2018
Copy link
Member

@fgalan fgalan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dcalvoalonso should I merge? I'm asking explictly because travis fails on this PR (but I'm not sure if it something that is going to be fixed in a next PR after this one)

@dcalvoalonso
Copy link
Contributor Author

@fgalan , let's merge first #121. I think that it will solve these errors.

@dcalvoalonso
Copy link
Contributor Author

@fgalan , with the last changes I have introduced, unit tests are passed.

@fgalan fgalan merged commit f97f62b into telefonicaid:master Mar 19, 2018
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.

2 participants