Skip to content

Adding GetBulkSecret API.#452

Merged
wcs1only merged 1 commit into
dapr:masterfrom
artursouza:get_bulk_secrets
Jan 25, 2021
Merged

Adding GetBulkSecret API.#452
wcs1only merged 1 commit into
dapr:masterfrom
artursouza:get_bulk_secrets

Conversation

@artursouza

Copy link
Copy Markdown
Contributor

Description

Please explain the changes you've made

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #421

Also includes #442

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@artursouza

Copy link
Copy Markdown
Contributor Author

This PR depends on dapr/components-contrib#632. Which means that DAPR_REF will need to be updated after the fix is in dapr/dapr.

@mukundansundar mukundansundar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm. can be merged after the componenets contrib change and dapr ref change.

@@ -1927,6 +1928,74 @@ public void getSecretsWithMetadata() {
assertEquals(expectedValue, result.get(expectedKey));
}

@wcs1only wcs1only Jan 25, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I had created a test for nullable etag, if you'd like to include it:

Suggested change
@Test
public void saveStateTestNullEtag() {
String key = "key1";
String etag = null;
String value = "State value";
ArgumentCaptor<DaprProtos.SaveStateRequest> argument = ArgumentCaptor.forClass(DaprProtos.SaveStateRequest.class);
doAnswer((Answer<Void>) invocation -> {
StreamObserver<Empty> observer = (StreamObserver<Empty>) invocation.getArguments()[1];
observer.onNext(Empty.getDefaultInstance());
observer.onCompleted();
return null;
}).when(daprStub).saveState(argument.capture(), any());
StateOptions options = buildStateOptions(StateOptions.Consistency.STRONG, StateOptions.Concurrency.FIRST_WRITE);
Mono<Void> result = client.saveState(STATE_STORE_NAME, key, etag, value, options);
result.block();
assertFalse(argument.getValue().getStates(0).hasEtag());
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually, why don't I just create a separate PR with the tests, cause I wrote one for deleteState as well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ack.

@wcs1only wcs1only mentioned this pull request Jan 25, 2021
3 tasks

@wcs1only wcs1only left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, pending ITs.

@codecov

codecov Bot commented Jan 25, 2021

Copy link
Copy Markdown

Codecov Report

Merging #452 (6095566) into master (57345eb) will decrease coverage by 0.35%.
The diff coverage is 76.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #452      +/-   ##
============================================
- Coverage     83.09%   82.73%   -0.36%     
- Complexity      904      929      +25     
============================================
  Files            77       79       +2     
  Lines          2644     2717      +73     
  Branches        264      269       +5     
============================================
+ Hits           2197     2248      +51     
- Misses          318      334      +16     
- Partials        129      135       +6     
Impacted Files Coverage Δ Complexity Δ
...k/src/main/java/io/dapr/client/DaprClientHttp.java 90.37% <61.90%> (-2.22%) 88.00 <4.00> (+4.00) ⬇️
.../src/main/java/io/dapr/client/DaprClientProxy.java 71.92% <66.66%> (-0.30%) 35.00 <2.00> (+2.00) ⬇️
...k/src/main/java/io/dapr/client/DaprClientGrpc.java 82.33% <75.00%> (-0.65%) 100.00 <6.00> (+6.00) ⬇️
...c/main/java/io/dapr/client/AbstractDaprClient.java 88.88% <80.00%> (-0.67%) 36.00 <3.00> (+3.00) ⬇️
...apr/client/domain/GetBulkSecretRequestBuilder.java 83.33% <83.33%> (ø) 4.00 <4.00> (?)
...va/io/dapr/client/domain/GetBulkSecretRequest.java 100.00% <100.00%> (ø) 7.00 <7.00> (?)
sdk/src/main/java/io/dapr/utils/Retry.java 50.00% <0.00%> (-25.00%) 1.00% <0.00%> (-1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57345eb...907748f. Read the comment docs.

@wcs1only wcs1only merged commit 233c699 into dapr:master Jan 25, 2021
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.

Implement getBulkSecret API

3 participants