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

Rebase of PR #775 "configGroups" literal in configuration group operations URL #780

Closed
wants to merge 72 commits into from

Conversation

Jagatjot
Copy link
Contributor

No description provided.

@Jagatjot Jagatjot changed the title Update deviceGroupAdministrationServer.js Rebase of PR https://github.com/telefonicaid/iotagent-node-lib/pull/775 Apr 24, 2019
@Jagatjot Jagatjot changed the title Rebase of PR https://github.com/telefonicaid/iotagent-node-lib/pull/775 Rebase of PR #775 "cgroups" literal in configuration group operations URL Apr 24, 2019
@Jagatjot
Copy link
Contributor Author

Jagatjot commented Apr 24, 2019

@fgalan This PR is rebased version of #775 which fixes issue #752

@@ -268,20 +268,40 @@ function loadContextRoutes(router) {
restUtils.checkRequestAttributes('headers', mandatoryHeaders),
restUtils.checkBody(templateGroup),
handleCreateDeviceGroup);

router.post('/iot/cgroups',
Copy link
Member

@fgalan fgalan Apr 29, 2019

Choose a reason for hiding this comment

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

Originally I saw this implemented in a more compact way, i.e. using

router.post('/iot/:services?/:cgroups?'

Why this was changed? It's ok also in the current way, but I'm curious about it...

Copy link
Member

Choose a reason for hiding this comment

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

(Btw, after some internal discussion, we probably would suggest a different literal for the URL? cgroups -> configGroups. However, please wait for confirmation before start doing the changes in the PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was changed to increase the test coverage which was decreased previously. OK will wait for your confirmation whether to keep cgroups or configGroups.

Copy link
Member

Choose a reason for hiding this comment

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

I do confirm the chosen token is configGroups. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Fixed at commit e6cee6a

@@ -301,6 +389,41 @@ describe('IoT Manager autoregistration', function() {
});
});
});
describe('When a new service is created in the IoT Agent', function() {
Copy link
Member

Choose a reason for hiding this comment

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

I assume that for each new added test for /iot/cgroups there is a test for /iot/services that has been copied and adapter to cover the new URL. Is my assumption 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, after adding the new functionality I had to ensure the previous functionality doesn't impact therefore similar test cases for /iot/cgroups are added.

Copy link
Member

Choose a reason for hiding this comment

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

So there is a 1:1 correspondence between the old and the new test, isn't it?

In that case, I'd suggest adding a mark in a comment just above the describe() of the old test with a text like this:

// This test will be removed if at the end the /iot/services API (now deprecated) is removed

Copy link
Member

Choose a reason for hiding this comment

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

Fixed at commit e6cee6a

README.md Outdated
@@ -717,7 +717,7 @@ The IoT Agents provide two means to define those device groups:
- Static **Type Configuration**: configuring the `ngsi.types` property in the
Copy link
Member

Choose a reason for hiding this comment

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

Please add a line to CHANGES_NEXT_RELEASE about this PR fixes. For instance:

Add: /iot/cgroups API endpoints (as equivalent to /iot/services) (#752)

Copy link
Contributor Author

@Jagatjot Jagatjot Apr 29, 2019

Choose a reason for hiding this comment

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

Fixed by ee0d509

@fgalan
Copy link
Member

fgalan commented May 22, 2019

I have realized that some changes in the payload would be needed. I mean the old operation is like this:

POST /iot/services
{
  "services": [
    {
      "apikey": "{{UL_APIKEY}}",
      "entity_type": "device",
      …
      "protocol": [ "IoTA-UL" ]
    }
  ]
}

So the new one should be:

POST /iot/configGroups
{
  "configGroups": [
    {
      "apikey": "{{UL_APIKEY}}",
      "entity_type": "device",
      …
      "protocol": [ "IoTA-UL" ]
    }
  ]
}

But as far as I understand, with the current PR code (cconfig -> configGroups change appart) is like this:

POST /iot/configGroups
{
  "services": [
    {
      "apikey": "{{UL_APIKEY}}",
      "entity_type": "device",
      …
      "protocol": [ "IoTA-UL" ]
    }
  ]
}

I mean, URl is using configGroup and payload uses service. This should be fixed.

(I have put my example with POST, but of course, this applies to other operations, e.g. GET, etc.)

@fgalan
Copy link
Member

fgalan commented May 30, 2019

@Jagatjot any update in this PR, please? Don't hesitate of asking if something is not clear about what is pending in this PR to be finished and merged. Thanks!

@Jagatjot
Copy link
Contributor Author

@fgalan Fixed by f90ddfb

Add: support for authentication to NGSI subscription requests (#592)
Fix: process dies if reconnection to DB fails instead of remain in a zombie state (#772)
Add: handlers defined with setConfigurationHandler receive as argument the service, subservice, resource and apikey (#769)
Add: /iot/configGroups API endpoints (as equivalent to /iot/services) (#752)
Copy link
Member

Choose a reason for hiding this comment

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

Similar to telefonicaid/iotagent-json#400, only the lines corresponding to this PR should be added in this PR. In particular, only this one:

Add: /iot/configGroups API endpoints (as equivalent to /iot/services) (#752)

Copy link
Member

Choose a reason for hiding this comment

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

Fixed. CHANGES_NEXT_RELEASE looks correct at commit e6cee6a

@Jagatjot
Copy link
Contributor Author

Sorry for the delay... I have done a new review and provide a new round of feedback. Thanks for your contribution!

Many of the new comments can be directly applied using the "commit suggestion" button. The PR is progressing and that's good although some changes are required in the tests part.

@fgalan Thank you for review. I have added a new set of implementation and the test cases. Please review and re-run the travis as it didn't ran while executing the commits.

@Jagatjot Jagatjot requested a review from fgalan June 22, 2020 14:52
@fgalan
Copy link
Member

fgalan commented Jun 22, 2020

Please review and re-run the travis as it didn't ran while executing the commits.

Please solve conflict on CHANGES_NEXT_RELEASE. Travis doesn't run if the PR has conflicts in place. Once solved, travis will be triggered automatically.

@fgalan
Copy link
Member

fgalan commented Oct 7, 2020

I understand you have closed this PR as you have left FIWARE activities :(

I think it would be a good idea to keep it so your work not get lost and maybe others can continue it in the future. In that line, I have ported this PR to #922. However, if you find this is not appropiated (in the case you want to completely withdraw your work from the repository) please tell me and I will close #922.

@Jagatjot
Copy link
Contributor Author

Jagatjot commented Oct 7, 2020

I understand you have closed this PR as you have left FIWARE activities :(

I think it would be a good idea to keep it so your work not get lost and maybe others can continue it in the future. In that line, I have ported this PR to #922. However, if you find this is not appropiated (in the case you want to completely withdraw your work from the repository) please tell me and I will close #922.

@fgalan It will be fine to keep the PR #922 open as it will not waste the work done till now :)

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.

None yet

2 participants