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

[Do not merge] New config groups API endpoint #1637

Closed
wants to merge 35 commits into from

Conversation

mapedraza
Copy link
Collaborator

@mapedraza mapedraza commented Aug 7, 2024

Issue #752
Overpasses #1375

Kudos to: @KeshavSoni2511

Pending:

Just some minor improvements points:

1): I can't see tests implementing cross functionality. I mean, creating groups using services and then retrieving it through configGroup endpoint. It should be aded in both ways, I mean, creating as services and configGroup and retrieving them with the opposite.
2) In order to configure the keyword used by the new endpoint, it would be great if instead of having a magic word, it is moved to the constans file (lib/constants.js), replacing each time in the code appears 'configgroups'. It should be configured for the API route and for the term used in the json response.

Check before merging:

  • Modify just merged doc/getting-started.md to use the new API (Done here: cdb791c)

@mapedraza mapedraza changed the title New config groups API endpoint [WIP] New config groups API endpoint Aug 7, 2024
@mapedraza
Copy link
Collaborator Author

Commit aa64573 solves pending point 2:

  1. In order to configure the keyword used by the new endpoint, it would be great if instead of having a magic word, it is moved to the constans file (lib/constants.js), replacing each time in the code appears 'configgroups'. It should be configured for the API route and for the term used in the json response.

Tests use a constants strings (http://localhost:4041/iot/configGroups) instead of a parameter (constants.CONFIGGROUP_TERM) to be more legible

@mapedraza
Copy link
Collaborator Author

Commit 0b38688 solves pending point 1):

1): I can't see tests implementing cross functionality. I mean, creating groups using services and then retrieving it through configGroup endpoint. It should be aded in both ways, I mean, creating as services and configGroup and retrieving them with the opposite.

@mapedraza mapedraza changed the title [WIP] New config groups API endpoint New config groups API endpoint Aug 9, 2024
Copy link
Member

Choose a reason for hiding this comment

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

This file should not be part of repository.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed in e2ea6c2
Added to gitignore here: d48cc4b

Copy link
Member

Choose a reason for hiding this comment

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

Bidirectional feature was not removed on previous PRs ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added this PR with more leftovers: #1644

Once it gets merged, this PR should be sync

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in: 611b45f

@@ -3,4 +3,5 @@
- Fix: service header to use uppercase in case of update and delete (#1528)
- Fix: Allow to send to CB batch update for multimeasures for NGSI-LD (#1623)
- Add: new JEXL transformations for including into an array keys that have a certain value: valuePicker and valuePickerMulti
- 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.

Suggested change
- Add /iot/configGroups API endpoints (as equivalent to /iot/services) (#752)
- Add /iot/configGroups API endpoints (as equivalent to /iot/services) (#752)
- Deprecated: /iot/services API routes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added here:e1f4e47

This commit also includes an entry in deprecated.md file

doc/api.md Outdated
@@ -1847,11 +1850,11 @@ _**Response headers**_

Successful operations return `Content-Type` header with `application/json` value.

#### Modify config group `PUT /iot/services`
#### Modify config group `PUT /iot/configGroups` or `PUT /iot/services`
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 remove references to /iot/services in any file, as it is deprecated feature and, as stated in doc

Documentation on deprecated features is removed from the repository documentation. Documentation is still available in the documentation set associated to older versions (in the repository release branches).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pending to choose the final term for configGroups

Comment on lines 29 to 30
- Services API routes (`/iot/services`) in favor of the `/iot/configGroups`. Both are still supported, but the former
is deprecated.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Services API routes (`/iot/services`) in favor of the `/iot/configGroups`. Both are still supported, but the former
is deprecated.
- Services API routes (`/iot/services`) in favor of the `/iot/configGroups`.

Looking the above elements, it is assumed that every deprecated feature is still supported, except the ones with the "(finally removed in x)" part.

Copy link
Member

@AlvaroVega AlvaroVega left a comment

Choose a reason for hiding this comment

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

LGTM

})
],
callback
);
Copy link
Member

Choose a reason for hiding this comment

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

try to avoid "Linter changes" not corresponding to current PR.

@mapedraza mapedraza changed the title New config groups API endpoint [Do no merge] New config groups API endpoint Aug 23, 2024
@fgalan
Copy link
Member

fgalan commented Aug 23, 2024

After @mrutid @mapedraza @fgalan discussion today, this PR is going to be re-worked.

@mapedraza mapedraza changed the title [Do no merge] New config groups API endpoint [Do not merge] New config groups API endpoint Aug 23, 2024
@mapedraza mapedraza mentioned this pull request Aug 23, 2024
3 tasks
@mapedraza
Copy link
Collaborator Author

overpassed by #1648

@mapedraza mapedraza closed this Aug 26, 2024
@fgalan fgalan deleted the prelanding/new-config-groups branch August 26, 2024 14:18
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.

6 participants