Skip to content

Conversation

jason-fox
Copy link
Contributor

@jason-fox jason-fox commented Nov 26, 2020

This PR adds an additional "mixed" mode to the IoT Agent.

  1. Flow of control of existing IoT Agent- NGSI v2 context brokers is not changed (e.g. Orion Classic)
  2. Flow of control of existing IoT Agent- NGSI-LD context brokers is not changed (e.g Stellio, Scorpio)
  3. "Mixed" mode is a compromise, suggested here and committed to raising as a separate PR here

@fgalan fgalan mentioned this pull request Nov 26, 2020
@jason-fox
Copy link
Contributor Author

jason-fox commented Nov 26, 2020

Includes the usual Documentation and Unit Test Changes and appropriate code coverage.

Before reviewing, I'd suggest actioning #943 first so the builds aren't queuing on Travis for hours before each suggested change. I think Travis is also causing the timeout of one test.

@fgalan
Copy link
Member

fgalan commented Nov 27, 2020

Before reviewing, I'd suggest actioning #943 first so the builds aren't queuing on Travis for hours before each suggested change. I think Travis is also causing the timeout of one test.

Done. Please sync the PR with master to get the changes.

doc/api.md Outdated
| `explicitAttrs` | `explicitAttrs` | Boolean value to support selective ignore of measures for device so that IOTA doesn’t progress. If not specified default is false. | `true/false` |

| `explicitAttrs` | `explicitAttrs` | Boolean value to support selective ignore of measures for device so that IOTA doesn’t progress. If not specified default is `false`. | `true/false` |
| `ngsiVersion` | `ngsiVersion` | optional string value used in mixed mode to switch between **NGSI-v2** and **NGSI-LD**payloads. The default is `v2`. | `v2/ld/mixed` |
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 markdown rendering is broken here.

imagen

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 : 9ad9268

doc/api.md Outdated
| `explicitAttrs` | `explicitAttrs` | optional boolean value, to support selective ignore of measures so that IOTA doesn’t progress. If not specified default is false. |
| `expressionLanguage` | `expresionLanguage` | optional boolean value, to set expression language used to compute expressions, possible values are: legacy or jexl. When not set or wrongly set, `legacy` is used as default value. |
| `explicitAttrs` | `explicitAttrs` | optional boolean value, to support selective ignore of measures so that IOTA doesn’t progress. If not specified default is `false`. |
| `ngsiVersion` | `ngsiVersion` | optional string value used in mixed mode to switch between **NGSI-v2** and **NGSI-LD** payloads. The default is `v2`.
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to enumerate the possible values this field can take.

Copy link
Member

Choose a reason for hiding this comment

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

In addition, I'd suggest to add the following:

If IOTA doesn't run in mixed mode, then this field is ignored.

If I have understood correctly what mixed mode means :)

Copy link
Contributor Author

@jason-fox jason-fox Nov 30, 2020

Choose a reason for hiding this comment

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

Fixed 40b5256

}
```

- If you want to support a "mixed" mode with both **NGSI-v2** and **NGSI-LD** (experimental):
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to add an explanation of what mixed mode means. For instance:

In mixed mode the NGSI version to use is chosen at group or device provisioning time, instead of using an static setting for that. The ngsiVersion field in the provisioning API is used to specify this, at both group at device level (the device level overriding the group setting).

It is just an example, it can be improved, of course.

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 52cb089

doc/api.md Outdated
| `expressionLanguage` | `expresionLanguage` | optional boolean value, to set expression language used to compute expressions, possible values are: legacy or jexl. When not set or wrongly set, legacy is used as default value. |
| `explicitAttrs` | `explicitAttrs` | Boolean value to support selective ignore of measures for device so that IOTA doesn’t progress. If not specified default is false. | `true/false` |
| `explicitAttrs` | `explicitAttrs` | Boolean value to support selective ignore of measures for device so that IOTA doesn’t progress. If not specified default is `false`. | `true/false` |
| `ngsiVersion` | `ngsiVersion` | optional string value used in mixed mode to switch between **NGSI-v2** and **NGSI-LD**payloads. The default is `v2`. | `v2/ld/mixed` |
Copy link
Member

Choose a reason for hiding this comment

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

I see v2/ld/mixed as possible values. However, what does "mixed" means in this API? Or maybe it's a typo and only "v2/ld" are possible?

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 e142485

doc/api.md Outdated
| `expressionLanguage` | `expresionLanguage` | optional boolean value, to set expression language used to compute expressions, possible values are: legacy or jexl. When not set or wrongly set, legacy is used as default value. |
| `explicitAttrs` | `explicitAttrs` | Boolean value to support selective ignore of measures for device so that IOTA doesn’t progress. If not specified default is false. | `true/false` |
| `explicitAttrs` | `explicitAttrs` | Boolean value to support selective ignore of measures for device so that IOTA doesn’t progress. If not specified default is `false`. | `true/false` |
| `ngsiVersion` | `ngsiVersion` | optional string value used in mixed mode to switch between **NGSI-v2** and **NGSI-LD**payloads. The default is `v2`. | `v2/ld`. |
Copy link
Member

Choose a reason for hiding this comment

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

As we did with the device API I think "When not running in mixed mode, this field is ignored." should be added here.

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 1a4d4fd

| `cbHost` | `cbHost` | Context Broker connection information. This options can be used to override the global ones for specific types of devices. |
| `lazy` | `lazy` | list of common lazy attributes of the device. For each attribute, its `name` and `type` must be provided. |
| `commands` | `commands` | list of common commands attributes of the device. For each attribute, its `name` and `type` must be provided, additional `metadata` is optional. |
| `attributes` | `attributes` | list of common active attributes of the device. For each attribute, its `name` and `type` must be provided, additional `metadata` is optional. |
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to add the mixed mode to the NGSI-LD feature sublist in CHANGES_NEXT_RELEASE. I mean:

Add basic NGSI-LD support as experimental feature (#842)
...
- Commands
- Mixed mode (based in ngsiVersion field in the provisioning API)

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 3dee480

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 2835d6d into telefonicaid:master Dec 2, 2020
@jason-fox jason-fox deleted the feature/mixed branch February 5, 2021 13:43
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