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.

Sign up for GitHub

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

Jump to bottom

FIX - Replace MongoClientOptions to MongoClientUri in mongoClient of Mongo/STHSinks #2387 #2402

Merged
merged 7 commits into from
Aug 30, 2024
Merged

FIX - Replace MongoClientOptions to MongoClientUri in mongoClient of Mongo/STHSinks #2387 #2402

merged 7 commits into from
Aug 30, 2024

Conversation

kumardeepak5
Copy link
Contributor

@kumardeepak5 kumardeepak5 commented Aug 22, 2024 •
edited by AlvaroVega
Loading

@AlvaroVega , PR is ready to review.

Edit: issue #2402

Related issue #2387

@AlvaroVega
Copy link
Member

This PR sounds goods to me, but not sure we can introduce this breaking change keeping old options (mongo_hosts, mongo_username, mongo_password, mongo_auth_source, mongo_replica_set) at the first time and removing then in a future.

@fgalan
Copy link
Member

fgalan commented Aug 22, 2024

This PR sounds goods to me, but not sure we can introduce this breaking change keeping old options (mongo_hosts, mongo_username, mongo_password, mongo_auth_source, mongo_replica_set) at the first time and removing then in a future.

I agree. Similar to what we do in Orion, deprecating mongo CLI options in release 3.12.0 (but still supported) and removing definitively in a next version 4.0.0.

In this sense:

  • It's ok to remove that options from documentation (following the rule for deprecated features: "Documentation on deprecated features is removed from the repository documentation. Documentation is still available in the documentation set associated to older versions")
  • Recover the support to that options in the code. In the case of using the new mongo_uri setting, the old mongo_ options are ignored.
  • Add the following lines to CHANGES_NEXT_RELEASE
[cygnus-common][cygnus-ngsi] New setting mongo_uri (#2402)
[cygnus-common][cygnus-ngsi] Deprecate (mongo_hosts, mongo_username, mongo_password, mongo_auth_source, mongo_replica_set) (use mongo_uri instead)

@kumardeepak5
Copy link
Contributor Author

@AlvaroVega, @fgalan Please Review PR.

fgalan
fgalan reviewed Aug 22, 2024
View reviewed changes
CHANGES_NEXT_RELEASE Outdated
@@ -1 +1,3 @@
- [cygnus-ngsi] Upgrade Debian version from 12.5 to 12.6 in Dockerfile
- [cygnus-common][cygnus-ngsi] New setting mongo_uri (#2402)
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
- [cygnus-common][cygnus-ngsi] New setting mongo_uri (#2402)
- [cygnus-common][cygnus-ngsi] New setting mongo_uri (#2402, #2387)

@AlvaroVega is ok that suggestion?

@kumardeepak5 please wait to @AlvaroVega confirmation before applying the suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

#2402 is a PR not a issue
Related issue with this PR is #2387

CNR entry should be: [cygnus-common][cygnus-ngsi] New setting mongo_uri (#2387)

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in 85e58fb

@kumardeepak5
Update CHANGES_NEXT_RELEASE
85e58fb 
Modified CNR Entry As per Review Comment
fgalan
fgalan reviewed Aug 30, 2024
View reviewed changes
doc/cygnus-ngsi/flume_extensions_catalogue/ngsi_mongo_sink.md Outdated
@@ -314,6 +314,7 @@ When datamodel changes Cygnus tries to recreate index (delete current and create
| data\_model | no | dm-by-entity | <i>dm-by-service-path</i>, <i>dm-by-entity</i> or <dm-by-attribute</i>. <i>dm-by-service</i> is not currently supported. |
| attr\_persistence | no | row | <i>row</i> or <i>column</i>. |
| attr\_metadata\_store | no | false | <i>true</i> or <i>false</i>. |
| mongo\_uri | no | <i>empty</i> | Mongo DB Connection String. In case of non empty mongo\_uri parameters (mongo\_hosts, mongo\_username, mongo\_password, mongo\_auth_source, mongo\_replica_set) would be ignored. |
| mongo\_hosts | no | localhost:27017 | FQDN/IP:port where the MongoDB server runs (standalone case) or comma-separated list of FQDN/IP:port pairs where the MongoDB replica set members run. |
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
| mongo\_hosts | no | localhost:27017 | FQDN/IP:port where the MongoDB server runs (standalone case) or comma-separated list of FQDN/IP:port pairs where the MongoDB replica set members run. |
| mongo\_hosts | no | localhost:27017 | **DEPRECATED** (use mongo_uri instead). FQDN/IP:port where the MongoDB server runs (standalone case) or comma-separated list of FQDN/IP:port pairs where the MongoDB replica set members run. |

Please add the same for all the other deprecated fields.

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 commit 016d5eb

AlvaroVega
AlvaroVega approved these changes Aug 30, 2024
View reviewed changes
Copy link
Member

@AlvaroVega AlvaroVega left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM and passing ball to @fgalan

fgalan
fgalan approved these changes Aug 30, 2024
View reviewed changes
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. Thanks for your contribution!

@fgalan fgalan merged commit 808bf1f into telefonicaid:master Aug 30, 2024
7 checks passed
@fgalan fgalan mentioned this pull request Aug 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fgalan fgalan fgalan approved these changes

@AlvaroVega AlvaroVega AlvaroVega approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kumardeepak5 @AlvaroVega @fgalan