Find source text repos in port and clone them #198
Conversation
Avoid using hash sign for url (#196) Co-Authored-By: Maxim <[email protected]>
There was a problem hiding this comment.
Reviewable status: 0 of 21 files reviewed, 3 unresolved discussions (waiting on @mXaln)
fetcher-web/src/main/kotlin/org/bibletranslationtools/fetcher/impl/repository/PrimaryRepoRepositoryImpl.kt line 8 at r2 (raw file):
import org.bibletranslationtools.fetcher.repository.PrimaryRepoRepository class PrimaryRepoRepositoryImpl : PrimaryRepoRepository {
To avoid confusion with repeating the same word, could we rename to PrimaryRepoAccessor or ContentRepoAccessor?
fetcher-web/src/main/kotlin/org/bibletranslationtools/fetcher/impl/repository/StorageAccessImpl.kt line 69 at r2 (raw file):
} override fun getReposRoot(): File {
could we name it getReposDir()?
fetcher-web/src/main/kotlin/org/bibletranslationtools/fetcher/usecase/CloneRemoteRepo.kt line 11 at r2 (raw file):
private val logger = LoggerFactory.getLogger(javaClass) fun cloneRepo(
we could move this to the PrimaryRepo__ API, since we already have getRepoUrl() in there. downloadRepo() would be preferred
There was a problem hiding this comment.
Reviewable status: 0 of 25 files reviewed, 3 unresolved discussions (waiting on @AnonymousWalker)
fetcher-web/src/main/kotlin/org/bibletranslationtools/fetcher/impl/repository/StorageAccessImpl.kt line 69 at r2 (raw file):
Previously, AnonymousWalker (Tony) wrote…
could we name it
getReposDir()?
Done.
fetcher-web/src/main/kotlin/org/bibletranslationtools/fetcher/usecase/CloneRemoteRepo.kt line 11 at r2 (raw file):
Previously, AnonymousWalker (Tony) wrote…
we could move this to the PrimaryRepo__ API, since we already have
getRepoUrl()in there.downloadRepo()would be preferred
Done. By refactoring to use SourceCacheAccessor
fetcher-web/src/main/kotlin/org/bibletranslationtools/fetcher/impl/repository/PrimaryRepoRepositoryImpl.kt line 8 at r2 (raw file):
Previously, AnonymousWalker (Tony) wrote…
To avoid confusion with repeating the same word, could we rename to
PrimaryRepoAccessororContentRepoAccessor?
Done. I have removed PrimaryRepoAccessor. Instead I came up to using your idea of cache accessor, that will fetch available repos for all languages in PORT on server start (takes up to 10 seconds). Then I'm going to use this cache to get availability status of the source texts for any language.
There was a problem hiding this comment.
Reviewable status: 0 of 25 files reviewed, 6 unresolved discussions (waiting on @mXaln)
fetcher-web/src/main/kotlin/org/bibletranslationtools/fetcher/impl/repository/SourceAvailabilityCacheAccessor.kt line 8 at r4 (raw file):
import org.slf4j.LoggerFactory class SourceAvailabilityCacheAccessor(
SourceTextAccessor
or
SourceTextRepository
fetcher-web/src/main/kotlin/org/bibletranslationtools/fetcher/impl/repository/SourceAvailabilityCacheAccessor.kt line 30 at r4 (raw file):
} override fun downloadRepo(repoName: String, repoUrl: String) {
this function doesn't seem to have any relationship with this class. You could consider moving it to the RC repo
fetcher-web/src/main/kotlin/org/bibletranslationtools/fetcher/impl/repository/SourceAvailabilityCacheAccessor.kt line 34 at r4 (raw file):
val process = ProcessBuilder() .command("git", "clone", repoUrl, repoName)
Fetcher web server doesn't seem to be a good place for using external process. The pipeline script using git clone is located outside of Fetcher webapp.
How do you think if we use HTTP request to download the repo instead?
fetcher-web/src/main/kotlin/org/bibletranslationtools/fetcher/impl/repository/PrimaryRepoRepositoryImpl.kt line 8 at r2 (raw file):
Previously, mXaln (Maxim) wrote…
Done. I have removed PrimaryRepoAccessor. Instead I came up to using your idea of cache accessor, that will fetch available repos for all languages in PORT on server start (takes up to 10 seconds). Then I'm going to use this cache to get availability status of the source texts for any language.
I agree on the idea. For your case, given this class with 1 method, you could just put this in your cache accessor. It's not necessary to have a dedicated builder that has a single method.
There was a problem hiding this comment.
Reviewable status: 0 of 26 files reviewed, 6 unresolved discussions (waiting on @AnonymousWalker)
fetcher-web/src/main/kotlin/org/bibletranslationtools/fetcher/impl/repository/SourceAvailabilityCacheAccessor.kt line 8 at r4 (raw file):
Previously, AnonymousWalker (Tony) wrote…
SourceTextAccessor
or
SourceTextRepository
Done.
fetcher-web/src/main/kotlin/org/bibletranslationtools/fetcher/impl/repository/SourceAvailabilityCacheAccessor.kt line 30 at r4 (raw file):
Previously, AnonymousWalker (Tony) wrote…
this function doesn't seem to have any relationship with this class. You could consider moving it to the RC repo
Done.
fetcher-web/src/main/kotlin/org/bibletranslationtools/fetcher/impl/repository/SourceAvailabilityCacheAccessor.kt line 34 at r4 (raw file):
Previously, AnonymousWalker (Tony) wrote…
Fetcher web server doesn't seem to be a good place for using external process. The pipeline script using
git cloneis located outside of Fetcher webapp.How do you think if we use HTTP request to download the repo instead?
I don't think that a web server can't use native git client. This makes it easier than doing http requests (first to fetch api to get default branch zip url, second, to download file) and then to unzip it. When git clone will do all of this in one call.
|
There was a problem hiding this comment.
Reviewable status: 0 of 26 files reviewed, 6 unresolved discussions (waiting on @mXaln)
This change is