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

Consolidate Neptune and RDS backends #7925

Merged
merged 6 commits into from
Aug 2, 2024

Conversation

bpandola
Copy link
Collaborator

@bpandola bpandola commented Aug 2, 2024

Despite being documented as two distinct services (with two distinct boto3 clients), the Neptune and RDS services share a single AWS backend (RDS). This is reflected in the botocore service definition for Neptune, which lists rds as its endpoint. Calling the DescribeDBClusters API method with either the neptune or rds boto3 client results in the exact same list of clusters--no filtering is done on the AWS backend. Neptune clusters are commingled with Aurora clusters.

@bblommers I think this makes more sense than trying to separate these services (and it prevents a lot of code duplication), but I want to get your opinion since you originally implemented/split out the Neptune service. (As I continue to work on improving the RDS backend overall, I do plan to handle some of the if self.engine == "neptune conditionals in a more elegant way.)

Copy link

codecov bot commented Aug 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.39%. Comparing base (bbcfc4c) to head (e5bbeab).
Report is 9 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7925      +/-   ##
==========================================
+ Coverage   94.38%   94.39%   +0.01%     
==========================================
  Files        1114     1109       -5     
  Lines       95224    95084     -140     
==========================================
- Hits        89875    89758     -117     
+ Misses       5349     5326      -23     
Flag Coverage Δ
servertests 29.13% <35.29%> (-0.03%) ⬇️
unittests 94.36% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bpandola bpandola requested a review from bblommers August 2, 2024 21:01
@bblommers
Copy link
Collaborator

@bpandola It is much cleaner like this, with RDS handling everything. If I remember correctly, I did try this approach first, but ran into some problems differentiating when a call came from Neptune and when from RDS. I don't remember exactly what went wrong, but based on this PR, it does work, so I'm more than happy to go ahead with this approach.

Copy link
Collaborator

@bblommers bblommers left a comment

Choose a reason for hiding this comment

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

Aside from the cache note, this looks good to me.

We would need to find a different approach for the implementation coverage script, to document that RDS coverage == Neptune coverage.
That doesn't have to be now, or even as part of this PR, just noting it as an FYI.

This allows the implementation coverage script to succeed for Neptune
@bpandola
Copy link
Collaborator Author

bpandola commented Aug 2, 2024

@bblommers

We would need to find a different approach for the implementation coverage script, to document that RDS coverage == Neptune coverage. That doesn't have to be now, or even as part of this PR, just noting it as an FYI.

Good catch. I did push an additional commit to fix the implementation coverage script (it was failing before). I think it should work fine now, with no additional work required.

@bpandola bpandola merged commit 7cd2763 into getmoto:master Aug 2, 2024
45 checks passed
@bpandola bpandola deleted the merge-neptune-with-rds branch August 2, 2024 22:47
Copy link
Contributor

github-actions bot commented Aug 2, 2024

This is now part of moto >= 5.0.12.dev64

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