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

Device provisioning using NGSIv2 #126

Merged

Conversation

dcalvoalonso
Copy link
Contributor

@dcalvoalonso dcalvoalonso commented Jun 14, 2018

This PR implements device provisioning using NGSIv2.

After the LGTM to telefonicaid/iotagent-node-lib#615 and the merge of telefonicaid/fiware-orion#3217. We can pass the tests against the CB.

@dcalvoalonso dcalvoalonso requested a review from fgalan June 29, 2018 12:25
@dcalvoalonso dcalvoalonso changed the title [WIP] Device provisioning using NGSIv2 Device provisioning using NGSIv2 Jun 29, 2018
@dcalvoalonso
Copy link
Contributor Author

Something interesting is that we have had to modify https://github.com/telefonicaid/lightweightm2m-iotagent/pull/126/files#diff-3e621782cff25b26014cd6896517f873 since in NGSIv2 attributes' names cannot include spaces. This is a problem with this agent since attributes can be imported from https://github.com/telefonicaid/lightweightm2m-iotagent/blob/master/omaRegistry.json and almost any attribute's name contains spaces...

@fgalan
Copy link
Member

fgalan commented Jul 2, 2018

Something interesting is that we have had to modify https://github.com/telefonicaid/lightweightm2m-iotagent/pull/126/files#diff-3e621782cff25b26014cd6896517f873 since in NGSIv2 attributes' names cannot include spaces. This is a problem with this agent since attributes can be imported from https://github.com/telefonicaid/lightweightm2m-iotagent/blob/master/omaRegistry.json and almost any attribute's name contains spaces...

What about URL scaping? E.g:

LWM2M Server URI -< LWM2M%20Server%20URI

a bit ugly, but will complian with NGSIv2 rules for attribute names...

@@ -188,10 +188,20 @@ function addUnsupportedAttributes(payload, configuration, deviceInformation, cal
if (omaRegistry[value.objectType].resources) {
Copy link
Member

Choose a reason for hiding this comment

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

CNR entry missing?

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 b17a3d4

type: 'string'
});

// In NGSIv2, spaces cannot be used in attributes' names. Therefore, we trim them.
Copy link
Member

Choose a reason for hiding this comment

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

Why this problem has arised when developing the NGSIv2-based provisioning but not in the previous NGSIv2-based active attributes values sending?

Or the situation was the same and also the same the solution applied (i.e. trim spaces)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because there is not any test as part of iotagent-node-lib, iotagent-ul or iotagent-json that contains spaces in the attributes' names... Maybe it is something that we must solve at iotagent-node-lib level?

Copy link
Member

Choose a reason for hiding this comment

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

If this something common to all the agents, iotagent-node-lib seems a good place to cover it with tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have been doing some tests regarding this issue and it is more complicated than it seemed initially.

If we decide that iotagent-node-lib will escape spaces in device's attributes automatically, this change may imply a big amount of work since it affects to device provisioning, forwarding of active attributes, plugins...

In my eyes, the best solution is:

Copy link
Member

Choose a reason for hiding this comment

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

Just to check if I'm understanding correctly...

You mean not touching iotagent-node-lib and implement space adaptation in the agent code itself? (either removing it as the PR shows now or apply %20 escaping). Is that 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.

Yes, that's correct. I agree with you to implementing %20 escaping but just in this specific IOTA. Implementing this in the iotagent-node-lib would be a very big effort and the benefit is very small...

Copy link
Member

Choose a reason for hiding this comment

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

I agree. So the idea is to change the current mechanism in this PR (removing spaces) to the new one (escaping spaces using %20)?

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, I will implement this ASAP. :)

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 9eadfbb

@fgalan
Copy link
Member

fgalan commented Jul 6, 2018

Is the issue created at Orion repository (telefonicaid/fiware-orion#3238) related with this PR? I mean, does that issue be debugged and eventually solved before merging this PR?

@dcalvoalonso
Copy link
Contributor Author

No, to avoid dependencies with telefonicaid/fiware-orion#3238, I have done a little workaround in https://github.com/dcalvoalonso/lightweightm2m-iotagent/blob/task/ngsiv2ContextAvailability/test/unit/ngsiv2/registration-test.js#297: to query the whole entity. :)

@@ -16,3 +16,4 @@
- Fix: first observable value is processed and forwarded to the CB (#73).
- Add: allow NGSIv2 for updating active attributes at CB, through configuration (#104)
- Using precise dependencies (~=) in packages.json
- Add: supports NGSIv2 for device provisioning (entity creation and context registration) at CB (#233)
Copy link
Member

Choose a reason for hiding this comment

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

It seems that #233 doesn't exists as an issue in this repo...

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 with 0196c46

type: 'string'
});

// In NGSIv2, spaces cannot be used in attributes' names. Therefore, we trim them.
Copy link
Member

Choose a reason for hiding this comment

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

Taking into account how it has been implemented at the end, I understand "... Therefore, we trim them" should be changed.

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 with 0196c46

@fgalan fgalan mentioned this pull request Jul 9, 2018
fgalan
fgalan previously approved these changes Jul 9, 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
Copy link
Contributor Author

As always, @fgalan thanks for your comments and detailed review, I really appreciate them! :)

We will wait for the results of the e2e tests in order to finish the PR and merge into master.

@fgalan
Copy link
Member

fgalan commented Aug 3, 2018

LGTM to changes 0196c46...a00bf45

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

@fgalan fgalan merged commit fca7d7c into telefonicaid:master Aug 3, 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