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.

Sign up for GitHub

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

Jump to bottom

chore: update gitlab repo remote url #354

Merged
merged 9 commits into from
Jul 16, 2023
Merged

chore: update gitlab repo remote url #354

merged 9 commits into from
Jul 16, 2023

Conversation

tromai
Copy link
Member

@tromai tromai commented Jul 7, 2023 •
edited
Loading

This PR does the following tasks:

  • For GitLab repositories, after we freshly clone the repo, we update the origin remote URL with the original non-token URL instead of the token-embedded URL. If a GitLab repository is already cloned, we don't do anything.
  • Bring the logic to checkout specific branch/commit inside GitService class to properly handle the remote URL (used for fetching) in each repository.
    • For GitLab repositories: It enables us to revert the origin remote URL back to the token-embedded URL to allow fetching new changes from remote. At the end of the checkout operation, the remote origin URL is once again updated to its original non-token URL.

TODO:

  • Add unit tests (We don't need integration tests as it relates to using personal GitLab tokens).

@tromai tromai self-assigned this Jul 7, 2023
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jul 7, 2023
@tromai tromai marked this pull request as ready for review July 7, 2023 09:24
@tromai tromai requested a review from behnazh-w as a code owner July 7, 2023 09:24
behnazh-w
behnazh-w reviewed Jul 12, 2023
View reviewed changes
src/macaron/errors.py Outdated
@@ -30,3 +30,7 @@ class ConfigurationError(MacaronError):

class CloneError(MacaronError):
"""Happens when cannot clone a git repository."""


class RepoError(MacaronError):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The relation between a RepoError and CloneError is not clear and they seem to have an overlap. RepoError seems to be related to checking out a repo, so I wonder if it should be renamed to something like CheckOutError? Or we could add a RepoError as a parent class of CloneError and CheckOutError.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will rename RepoError to RepoCheckOutError and update the doc string accordingly for clarity.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 38db651

src/macaron/slsa_analyzer/analyzer.py Show resolved Hide resolved
src/macaron/slsa_analyzer/analyzer.py Outdated
@@ -504,6 +504,7 @@ def _prepare_repo(
Git
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type here should be Git | None. Can you please fix it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 204d5f8.

src/macaron/slsa_analyzer/git_service/base_git_service.py Outdated
RepoError
If there is an error while checking out the specific branch or commit.
"""
raise NotImplementedError()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to raise the NotImplementedError. The abc module handles unimplemented abstract methods.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 73cd1bc

src/macaron/slsa_analyzer/git_service/base_git_service.py Outdated
Always raise, since this method should not be used to check out in any repository.
"""
raise RepoError(
f"Internal error when checking out branch {branch} and commit {digest} for repo {git_obj.project_name}."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of Internal error can you please be more specific. For example here the error should communicate that we cannot check out from an empty git service.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 77e4d14.

src/macaron/slsa_analyzer/git_service/gitlab.py Outdated
git_url.clone_remote_repo(clone_dir, clone_url)
repo = git_url.clone_remote_repo(clone_dir, clone_url)

# If ``git_url.clone_remote_repo`` return an Repo instance, this means that the repository is freshly cloned
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# If ``git_url.clone_remote_repo`` return an Repo instance, this means that the repository is freshly cloned
# If ``git_url.clone_remote_repo`` returns a Repo instance,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6678fe2

src/macaron/slsa_analyzer/git_service/gitlab.py Show resolved Hide resolved
src/macaron/slsa_analyzer/git_service/gitlab.py Outdated
try:
reconstructed_url = self.construct_clone_url(remote_origin_url)
except CloneError as error:
raise RepoError("Internal error prevent preparing the repo for the analysis.") from error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about being more specific about the error and avoiding Internal error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 77e4d14

src/macaron/slsa_analyzer/git_service/gitlab.py Outdated Show resolved Hide resolved
tests/slsa_analyzer/git_service/test_gitlab.py Show resolved Hide resolved
@tromai tromai merged commit 277ecfb into staging Jul 16, 2023
21 checks passed
@tromai tromai deleted the update-gitlab-repo-remote-url branch July 16, 2023 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behnazh-w behnazh-w behnazh-w approved these changes

Assignees

@tromai tromai

Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tromai @behnazh-w