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

Making refresh modifier public so that clients can do AOP failover #634

Merged
merged 1 commit into from Apr 6, 2017

Conversation

Projects
None yet
4 participants
@asarkar
Copy link
Contributor

commented Feb 9, 2017

No description provided.

@codecov-io

This comment has been minimized.

Copy link

commented Feb 9, 2017

Codecov Report

Merging #634 into master will not change coverage.

@@           Coverage Diff           @@
##           master     #634   +/-   ##
=======================================
  Coverage   72.19%   72.19%           
=======================================
  Files          70       70           
  Lines        2309     2309           
  Branches      322      322           
=======================================
  Hits         1667     1667           
  Misses        493      493           
  Partials      149      149
Impacted Files Coverage Δ
.../server/environment/JGitEnvironmentRepository.java 87.62% <ø> (ø)

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 1a839f9...2aabe26. Read the comment docs.

@spencergibb

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

Why do you need to do AOP on refresh()? It's only called from one place getLocations(), couldn't you do it there?

@asarkar

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2017

@spencergibb Not without extending AbstractScmAccessor, protected methods from which are used in getLocations.

@asarkar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2017

@spencergibb What does the team think? This is really a trivial change, so if you don't see a problem with it, I'd like to get it merged. It'd help the ambitious clients.

@spencergibb

This comment has been minimized.

Copy link
Member

commented Mar 1, 2017

@asarkar can you explain in more detail why AOP wouldn't work around getLocations() while it would around refresh()?

@asarkar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2017

It's not that getLocations can't be intercepted, that's not hard. The question is what the alternative implementation might be when we don't want to execute the original method. Currently getLocations uses protected methods from AbstractScmAccessor, thus making it harded to provided an alternative without having another subclass of AbstractScmAccessor.
If you have a suggestion for an alternative getLocations without subclassing AbstractScmAccessor, I can do that.

@spencergibb

This comment has been minimized.

Copy link
Member

commented Mar 2, 2017

I think it would be helpful to step back and see why you are trying to do this rather than implementation details. That would help drive how to achieve your purposes.

@asarkar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2017

I believe this #631 answers your question.

@spencergibb spencergibb merged commit 8c36922 into spring-cloud:master Apr 6, 2017

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
ci/pivotal-cla Thank you for signing the Contributor License Agreement!
Details
codacy/pr Good work! A positive pull request.
Details
codecov/patch Coverage not affected when comparing 1a839f9...2aabe26
Details
codecov/project 72.19% remains the same compared to 1a839f9
Details

@spencergibb spencergibb added this to the 1.3.0.RELEASE milestone Apr 6, 2017

@dendron8

This comment has been minimized.

Copy link

commented Oct 17, 2018

@asarkar do you have an example of how you used this to provide failover? did you failover to a native/local repo?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.