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

50 migrate to new sdk #51

Merged
merged 8 commits into from
Feb 7, 2022
Merged

50 migrate to new sdk #51

merged 8 commits into from
Feb 7, 2022

Conversation

emlaver
Copy link
Contributor

@emlaver emlaver commented Jan 27, 2022

Checklist

  • Tick to sign-off your agreement to the Developer Certificate of Origin (DCO) 1.1
  • Added tests for code changes or test/build only changes
  • Updated the change log file (CHANGES.md|CHANGELOG.md) or test/build only changes
  • Completed the PR template below:

Description

fixes #50
The underlying Cloudant client library is now EOL. This PR refactored the codebase and tests to use the new cloudant-java-sdk library.

Approach

Schema & API Changes

"No change"

Security and Privacy

Testing

Updated existing tests to use new library.
No new tests.

Monitoring and Logging

@emlaver emlaver self-assigned this Jan 27, 2022
@ricellis ricellis self-requested a review January 28, 2022 13:39
Copy link
Member

@ricellis ricellis left a comment

Choose a reason for hiding this comment

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

Some initial thoughts. The diff is (unnsurprisingly) quite challenging so maybe you can address these and then I can take another look.

.build();
Response<Ok> result = service.deleteDatabase(deleteDbOptions).execute();
if (!result.getResult().isOk()) {
LOG.error(String.format("Error during deletion of database %s. Error code: %d Error response: %s",
Copy link
Member

Choose a reason for hiding this comment

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

What is the motivation for logging in the test vs just failing?

Copy link
Contributor Author

@emlaver emlaver Jan 31, 2022

Choose a reason for hiding this comment

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

I removed this log and added an assertion in df22ab8

@emlaver emlaver force-pushed the 50-migrate-to-new-sdk branch from 45ef249 to df22ab8 Compare February 2, 2022 15:48
@emlaver emlaver marked this pull request as ready for review February 2, 2022 16:23
@emlaver emlaver requested a review from ricellis February 2, 2022 16:23
Copy link
Member

@ricellis ricellis left a comment

Choose a reason for hiding this comment

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

The copyright dates need a refresh frrom 2021 to 2022 and a couple of minor new questions, but otherwise this is looking OK to swap the client out ready for future improvements.

Copy link
Member

@ricellis ricellis left a comment

Choose a reason for hiding this comment

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

+1, a changelog nit sorry I didn't spot it earlier.
I think this will be ok to merge if you are satisified with your testing outcomes.

emlaver and others added 2 commits February 4, 2022 10:01
Co-authored-by: Rich Ellis <ricellis@users.noreply.github.com>
@emlaver emlaver merged commit 50d13f2 into master Feb 7, 2022
@ricellis ricellis added this to the 0.100.next milestone May 6, 2022
@emlaver emlaver deleted the 50-migrate-to-new-sdk branch May 27, 2022 14:29
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.

Migrate java client
2 participants