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

Update logging message on MultipleJGitEnvironmentRepository, JGitEnvironmentRepository #2162

Merged
merged 3 commits into from
Nov 8, 2022

Conversation

ndy2
Copy link
Contributor

@ndy2 ndy2 commented Sep 26, 2022

As decribe in #2161, it is more easy to debug by throw original exception.
It can be handled by adding original exception as suppressed to master branch exception in case both findOne try thorws exception.

Thanks for @woshikid to sharing solutions.

@pivotal-cla
Copy link

@ndy2 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@ndy2 Thank you for signing the Contributor License Agreement!

@codecov
Copy link

codecov bot commented Sep 26, 2022

Codecov Report

Base: 77.15% // Head: 77.52% // Increases project coverage by +0.36% 🎉

Coverage data is based on head (6054c2a) compared to base (bc8227c).
Patch coverage: 90.50% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##              3.1.x    #2162      +/-   ##
============================================
+ Coverage     77.15%   77.52%   +0.36%     
- Complexity     1455     1472      +17     
============================================
  Files           183      186       +3     
  Lines          5376     5442      +66     
  Branches        705      706       +1     
============================================
+ Hits           4148     4219      +71     
+ Misses          954      943      -11     
- Partials        274      280       +6     
Impacted Files Coverage Δ
...nfig/server/environment/EnvironmentController.java 91.95% <ø> (ø)
.../server/environment/JGitEnvironmentProperties.java 90.00% <ø> (-3.34%) ⬇️
...fig/server/ssh/PropertyBasedSshSessionFactory.java 80.00% <79.68%> (-3.88%) ⬇️
.../server/environment/JdbcEnvironmentRepository.java 78.33% <88.23%> (+7.96%) ⬆️
...ork/cloud/config/monitor/PropertyPathEndpoint.java 88.57% <100.00%> (-0.07%) ⬇️
...nfigServerBootstrapOverridesAutoConfiguration.java 100.00% <100.00%> (ø)
...onment/AwsParameterStoreEnvironmentRepository.java 100.00% <100.00%> (ø)
...server/environment/AwsS3EnvironmentRepository.java 89.65% <100.00%> (+0.12%) ⬆️
...t/HttpClientConfigurableHttpConnectionFactory.java 97.01% <100.00%> (+0.13%) ⬆️
.../server/environment/JGitEnvironmentRepository.java 87.62% <100.00%> (-0.28%) ⬇️
... and 18 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@woshikid
Copy link
Contributor

in JGitEnvironmentRepository#getLocations there is another Try Master Branch try catch. maybe we should do the same thing here.

@ndy2
Copy link
Contributor Author

ndy2 commented Sep 27, 2022

I will check and add !

@woshikid
Copy link
Contributor

I debugged and found the actually Try Master Branch behavior happens in JGitEnvironmentRepository#getLocations in normal situation.

@ndy2
Copy link
Contributor Author

ndy2 commented Sep 28, 2022

I found that there are actually three case where
tryMasterBranch flag in JGitEnvironmentRepository is used as flag for retry some logic

1. JGitEnvironmentRepository#getLocations
2. JGitEnvironmentRepository#checkoutDefaultBranchWithRetry
3. MultipleJGitEnvironmentRepository#findOne

two methods in JGitEnvironmentRepository handle original exception by logging.

Considering that, i think simple loggin on MultipleJGitEnvironmentRepository#findOne would be consistent solution for this issue.

So I add a log on MultipleJGitEnvironmentRepository#findOne refer to JGitEnvironmentRepository.

And fix a wrong log message in JGitEnvironmentRepository

@ndy2 ndy2 changed the title Update MultipleJGitEnvironmentRepository.findOne to add original exception as suppressed Update logging message on MultipleJGitEnvironmentRepository, JGitEnvironmentRepository Sep 28, 2022
@@ -160,7 +160,6 @@ public Environment findOne(String application, String profile, String label, boo
this.logger.debug("Cannot load configuration from " + candidate.getUri() + ", cause: ("
+ e.getClass().getSimpleName() + ") " + e.getMessage(), e);
}
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this being removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My IDE suggests

'continue' is unnecessary as the last statement in a loop 

so i removed it.
I find that there is one more such continue statement at MultipleJGitEnvironmentRepository.getLocations

May i remove 'continue;' from that also or retreive this one.

Copy link
Member

Choose a reason for hiding this comment

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

please limit changes to those specific to the issue.

@ndy2
Copy link
Contributor Author

ndy2 commented Nov 3, 2022

After I restore the continue statement.
BootstrapConfigServerIntegrationTests.contextLoads fails...

I have no clue that why it fails. Since the change i made is only to add some logging.
It successes in my local environment

@ryanjbaxter
Copy link
Contributor

@ndy2 can you please pull the latest changes from 3.1.x into your branch?

@ryanjbaxter ryanjbaxter merged commit 9ca431a into spring-cloud:3.1.x Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

suggestion on Exception Handling in MultipleJGitEnvironmentRepository
6 participants