implement jgit repository resolver #583
Conversation
laDok8
force-pushed
the
git_resolver
branch
from
f835c40
to
04e03fa
Compare
|
is the core really correct location? Wouldn't |
|
I'm not sure but all configurable properties (openshift,helm or XTF itself) seem to be in |
|
Looks like maven dependency |
laDok8
force-pushed
the
git_resolver
branch
from
2afe8cb
to
f482dc6
Compare
|
Can we have run from OS TS showing this dynamic resolver works as expected. That mean without those properties in configuration |
| return getRepositoryUrl(remoteConfig.getURIs().get(0).getHost(), pathTokens[0], pathTokens[1]); | ||
| } | ||
|
|
||
| static { |
There was a problem hiding this comment.
I'm not in favor to have this executed when it's not needed. Can the whole class be written as a singleton. It would be used like:
GitRemoteResolver gitRemoteResolver = GitRemoteResolver.getInstance(); // all initialization happens here
gitRemoteResolver.getRepositoryReference();
gitRemoteResolver.getRepositoryUrl();
There was a problem hiding this comment.
This code is not always executed, it's executed after loading to memory, which according to my testing happens upon first method call of mentioned class
There was a problem hiding this comment.
Ok, still I think that using Singleton would provide cleaner design.
There was a problem hiding this comment.
now the methods in Resolver are private, this would just move code you mentioned into GitRemoteConfig and would result in no public facing API change
There was a problem hiding this comment.
There would be no public facing change but internal code could use Singleton, Factory, or Strategy pattern to make it more maintanable. However I'm ok to let it be if we don't anticipate more complex use cases.
| */ | ||
| private static void resolveRepoFromHEAD() throws IOException, URISyntaxException { | ||
| //look for a git repository recursively till system root folder | ||
| Repository repository = new FileRepositoryBuilder().findGitDir().build(); |
There was a problem hiding this comment.
Would it make sense to use findGitDir(File current) instead and make current configurable?
There was a problem hiding this comment.
IMO not really, at this point user could specify xtf.git.repository.url which is also more universal than "hardcoding" local path
laDok8
force-pushed
the
git_resolver
branch
from
f482dc6
to
7681722
Compare
|
updated CI run with applied changes |
laDok8
force-pushed
the
git_resolver
branch
3 times, most recently
from
e339819
to
cabd7f3
Compare
| Repository repository = new FileRepositoryBuilder().findGitDir().build(); | ||
|
|
||
| if (repository == null) { | ||
| log.debug("Failed to find a git repository"); |
There was a problem hiding this comment.
can be these log into error instead of debug.. I would like to see a message if something is wrong. now you have to do extra step to enable debug logging to see what is wrong.
laDok8
force-pushed
the
git_resolver
branch
from
cabd7f3
to
56243a4
Compare
|
After discussion with @laDok8 the functionality is working. If any other changes are required, those will be handled in a follow up PR. Merging. |
Introduce
xtf.git.repository.urlandxtf.git.repository.refproperties intended for pointing to TS repo itself.This should get auto resolved via jgit, but can be overridden by manually setting properties
closes #574