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

Feature: Tests for http, https & ssh remotes #25

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

macmacal
Copy link

@macmacal macmacal commented Mar 14, 2023

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 called remote_url which is passed to the factory method.

It was tested both by running testes manually and by usage in my Gitea instance.

@theSage21
Copy link
Owner

Could you test out the latest develop branch? If that works for you we can close this PR.

@macmacal macmacal changed the title Feature: Support for ssh remotes Feature: Tests for http, https & ssh remotes Mar 27, 2023
@macmacal
Copy link
Author

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 for Repo class. In PR you will find changes to incorporate this idea.
  • The current naming convention of all these remotes 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 about branch and sha when we can (and we do) access it through repo. 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.

@theSage21
Copy link
Owner

I'll try and answer some of the queries:

  1. RemoteInfo can indeed be absorbed into the Repo 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 the Repo class. If you notice; the RemoteInfo is only used in the Remote implementations instead of the Repo implementation. This is because the local git can be independent of the requirements of the place you want to publish your reports.
  2. 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 and Github as Platform instead of Remote. Then a Repo can have Remote information and can generate a Report which can be published on a Platform. 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.
  3. 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. 🤔 I'll look into this more when I work on this project again.
  4. 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 with feature-1-myusername branch naming and to github with feature-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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants