jaypore_ci
jaypore_ci copied to clipboard
Feature: Tests for http, https & ssh remotes
As mentioned in #18 :
- This PR introduces support for ssh remotes by parsing the URLs into https,
- The
Repo
class has been extended with 3 private methods: for obtaining remote URL, for parsing & validation and for ssh-to-https conversion (these functionalities should be available for all kinds of Repo instances), - The pytest fixture
piepline
now includes another parameter calledremote_url
which is passed to thefactory
method.
It was tested both by running testes manually and by usage in my Gitea instance.
Could you test out the latest develop branch? If that works for you we can close this PR.
As far as I can see, you have already implemented a feature to parse & handle ssh remotes through RemoteInfo
class. I think your implementation based on string manipulation is more cleaner and readable than my regex one-liner. However, you haven't implement tests for that, so I decided to integrate our approaches.
Somethings to consider:
- In my eyes the
RemoteInfo
class looks like a member field dedicated forRepo
class. In PR you will find changes to incorporate this idea. - The current naming convention of all these
remote
s variables gave me many headaches. I suggest that some names should be more verbose. - The test suite
doc_examples
forced me to add an additional env variable (JAYPORECI_DOCS_EXAMPLES_TEST_MODE
) dedicated solely to mocking the remote url in the main code. - In
remotes/git.py
I don't really understand why we need to duplicate information aboutbranch
andsha
when we can (and we do) access it throughrepo
. Something must be simplified, but in this moment I have no foggiest idea what it could be.
return cls(
repo=repo,
branch=repo.branch,
sha=repo.sha,
)
I have tested the changes on my Ubuntu 22.04 setup. The unit tests gave me all greens inside container.
I'll try and answer some of the queries:
-
RemoteInfo
can indeed be absorbed into theRepo
class but I didn't want to do it for two reasons:- Use cases might arise where the remote parsing can depend on what the publishing platform needs. For example some platform may only need the names of the repos etc.
- I haven't yet done a lot of research on what kinds of use cases can come up while dealing with a single local and multiple remotes. Each remote platform can have it's own requirements and so I leave that decision to the person implementing the
Remote
class instead of absorbing that logic into theRepo
class. If you notice; theRemoteInfo
is only used in theRemote
implementations instead of theRepo
implementation. This is because the local git can be independent of the requirements of the place you want to publish your reports.
- I agree about the naming conventions :D If there are better names that you can suggest I'm happy to use those. One conflict I want to fix is to start calling
Gitea
andGithub
asPlatform
instead ofRemote
. Then aRepo
can haveRemote
information and can generate aReport
which can be published on aPlatform
. However I'm still looking for other ways to name things since what makes sense in my head often does not make sense to others. - I didn't understand this part. If you need to mock remotes you can add them to the subprocess mock file? I'm surprised this did not fail in the CI itself. :thinking: I'll look into this more when I work on this project again.
- The duplication is there since the local repo can have certain branch names and the push can be done to a separate branch. For example a local
feature-1
branch can be pushed to gitea withfeature-1-myusername
branch naming and to github withfeature-1-archive
naming. To support the case when CI needs to push this same branch to two separate upstreams with different names we do the duplication and don't automatically tie it up with whatever is the local remote/branch/sha.