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

refactor: solve the inconsistency of credentials refresh && add the m… #49

Merged
merged 1 commit into from
Jun 15, 2023

Conversation

yndu13
Copy link
Collaborator

@yndu13 yndu13 commented Jun 11, 2023

…ethod of asynchronous refresh

You need to complete
  • unit tests and/or feature tests
  • documentation is changed or added

@yndu13 yndu13 force-pushed the refresh-credential branch 2 times, most recently from a52c457 to c49b9f3 Compare June 12, 2023 06:43
@yndu13 yndu13 force-pushed the refresh-credential branch 6 times, most recently from 50b4c9a to db30f7d Compare June 12, 2023 13:20
public String STSEndpoint;
public String STSEndpoint = "sts.aliyuncs.com";
@NameInMap("asyncCredentialUpdateEnabled")
public boolean asyncCredentialUpdateEnabled = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

这个需要对外暴露吗

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

嗯嗯,现在的设计是支持阻塞、非阻塞两种刷新,非阻塞的刷新其实是推荐的

return new Builder();
}

public Credential setAccessKeyId(String accessKeyId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

有了 builder setXXX 是不是就不需要了。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

本应该不需要的,但是这个credential是Model,model的设计是setters进行配置,这个只能看看darabonba v2要不要去掉

@@ -61,7 +61,7 @@ public void createCredentialTest() {
response.setHttpContent(new String("{\"SessionAccessKey\":{\"Expiration\":\"2019-12-12T1:1:1Z\",\"SessionAccessKeyId\":\"test\"," +
"\"SessionAccessKeySecret\":\"test\"}}").getBytes(), "UTF-8", FormatType.JSON);
when(client.syncInvoke(ArgumentMatchers.<HttpRequest>any())).thenReturn(response);
Assert.assertTrue(provider.createCredential(client) instanceof RsaKeyPairCredential);
Assert.assertEquals(AuthConstant.RSA_KEY_PAIR, provider.createCredential(client).value().getType());
Copy link
Contributor

Choose a reason for hiding this comment

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

老的测试 是不是不该动啊

@@ -62,7 +62,8 @@
<dependency>
<groupId>com.aliyun</groupId>
<artifactId>tea</artifactId>
<version>1.0.6</version>
<version>1.2.7</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

跳度这么大么

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

嗯嗯,现在稳定版本是1.2.7

@yndu13 yndu13 force-pushed the refresh-credential branch 2 times, most recently from 91ef469 to 61a0773 Compare June 13, 2023 01:35
@yndu13 yndu13 force-pushed the refresh-credential branch 5 times, most recently from 9ce8075 to fbbe5fa Compare June 14, 2023 02:32
@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2023

Codecov Report

Merging #49 (fd3a66c) into master (6cd4708) will decrease coverage by 3.46%.
The diff coverage is 76.09%.

❗ Current head fd3a66c differs from pull request most recent head e9493f7. Consider uploading reports for the commit e9493f7 to get more accurate results

@@             Coverage Diff              @@
##             master      #49      +/-   ##
============================================
- Coverage     87.88%   84.42%   -3.46%     
- Complexity      457      482      +25     
============================================
  Files            39       44       +5     
  Lines          1304     1740     +436     
  Branches         94      111      +17     
============================================
+ Hits           1146     1469     +323     
- Misses          136      234      +98     
- Partials         22       37      +15     
Flag Coverage Δ
unittest 84.42% <76.09%> (-3.46%) ⬇️

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

Impacted Files Coverage Δ
...va/com/aliyun/credentials/AccessKeyCredential.java 100.00% <ø> (ø)
.../com/aliyun/credentials/BearerTokenCredential.java 100.00% <ø> (ø)
...ain/java/com/aliyun/credentials/Configuration.java 100.00% <ø> (ø)
...c/main/java/com/aliyun/credentials/Credential.java 75.00% <ø> (ø)
...ain/java/com/aliyun/credentials/StsCredential.java 100.00% <ø> (ø)
...iyun/credentials/http/CompatibleUrlConnClient.java 81.57% <ø> (-3.77%) ⬇️
...ain/java/com/aliyun/credentials/models/Config.java 16.98% <ø> (+4.98%) ⬆️
...edentials/provider/DefaultCredentialsProvider.java 81.25% <ø> (-3.13%) ⬇️
...ava/com/aliyun/credentials/utils/RefreshUtils.java 100.00% <ø> (ø)
...un/credentials/provider/URLCredentialProvider.java 40.22% <14.70%> (-14.16%) ⬇️
... and 19 more

... and 1 file with indirect coverage changes

@yndu13 yndu13 changed the title refactor: solve the inconsistency of credentials refresh && add the m… 【WIP】refactor: solve the inconsistency of credentials refresh && add the m… Jun 15, 2023
@yndu13 yndu13 changed the title 【WIP】refactor: solve the inconsistency of credentials refresh && add the m… refactor: solve the inconsistency of credentials refresh && add the m… Jun 15, 2023
@JacksonTian JacksonTian merged commit 844da20 into master Jun 15, 2023
@JacksonTian JacksonTian deleted the refresh-credential branch June 15, 2023 08:22
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.

4 participants