fix: do not pull the latest when analyzing a target with local repo path #125

tromai · 2023-03-28T04:49:23Z

Closes #117 .

tromai · 2023-03-28T06:48:20Z

Add tests for this fix.

… path Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>

Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>

tromai · 2023-03-31T01:43:40Z

This rebase is for adding the Signoff messages to the new commits in this branch.

behnazh-w · 2023-04-18T22:16:34Z

scripts/dev_scripts/integration_tests.sh

@@ -321,6 +321,54 @@ then
    log_fail
 fi

+echo -e "\n----------------------------------------------------------------------------------"
+echo "apache/maven: test not doing a git pull when analyzing a local cloned repo."


Suggested change

echo "apache/maven: test not doing a git pull when analyzing a local cloned repo."

echo "apache/maven: test not pulling from remote for a locally cloned repo."

Fixed in dc986c6

behnazh-w · 2023-04-18T22:50:55Z

src/macaron/slsa_analyzer/git_url.py

+    if not offline_mode:
+        if not digest or (digest and not commit_exists(git_obj, digest)):


Suggested change

if not offline_mode:

if not digest or (digest and not commit_exists(git_obj, digest)):

if not offline_mode and (not digest or not commit_exists(git_obj, digest):

Fixed in dc986c6

behnazh-w · 2023-04-18T22:53:14Z

src/macaron/slsa_analyzer/git_url.py

@@ -77,6 +76,9 @@ def check_out_repo_target(git_obj: Git, branch_name: str = "", digest: str = "")
    This method supports repositories which are cloned (full or shallow) from existing remote repositories.
    Other scenarios are not covered (e.g. a newly initiated repository).

+    If ``offline_mode`` is True. This method will not perform any pulling before checking out the branch


Suggested change

If ``offline_mode`` is True. This method will not perform any pulling before checking out the branch

If ``offline_mode`` is set, this method will not pull from remote while checking out the branch or commit.

Fixed in dc986c6

behnazh-w · 2023-04-18T23:01:12Z

src/macaron/slsa_analyzer/git_url.py

@@ -177,7 +176,12 @@ def check_out_repo_target(git_obj: Git, branch_name: str = "", digest: str = "")
            logger.info("Cannot find commit %s on branch %s. Please check the configuration.", digest, res_branch)
            return False

-    logger.info("Successfully checked out commit %s.", head_commit)
+    head_commit: Commit = git_obj.repo.head.commit
+    if not head_commit:


Shouldn't this check be done before comparing git_obj.repo.head.commit.hexsha with digest at line 159?

That is true. Within the if digest branch, we are not going to pull any new changes so it's okay for us to run this check before it.

Hmmm, after having a closer look. The reason I put this part here in the first place is that there are cases where Macaron checkouts the specific hash which update the head commit (line 162). This is why we only perform it after all checking out commit hash operations to make sure that the we could still extract the head commit for the analysis. But I think we still need that check before line 159 anyway

Fixed in dc986c6

But still we need to check if git_obj.repo.head.commit is not None here. Otherwise it might throw an exception.

If current_head is None here, should we return False early or should we proceed to try to check out the specific commit by the user 🤔 . The Exceptions for checking if digest exists or whether we can checkout digest (after here) should be caught and return False.

Add checks for the final HEAD commit after checking out the repository for edge cases. Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>

behnazh-w · 2023-04-18T23:40:24Z

src/macaron/slsa_analyzer/git_url.py

+    # We only pull the latest changes if we are not running in offline mode and:
+    #   - no digest is provided.
+    #   - or a commit digest is provided but it does not exist in the current local branch.
+    if not offline_mode and (not digest or (digest or not commit_exists(git_obj, digest))):


Why do you check digest again? We get to the second expression in the or statement only when not digest is False.

I agree. It could be removed as you mentioned.
if not offline_mode and (not digest or not commit_exists(git_obj, digest)):

Changed in e1e76d7.

Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>

tromai · 2023-04-19T04:25:43Z

src/macaron/slsa_analyzer/git_url.py

@@ -157,7 +154,10 @@ def check_out_repo_target(git_obj: Git, branch_name: str = "", digest: str = "")
            return False

    if digest:
-        if head_commit.hexsha == digest:
+        current_head: Commit = git_obj.repo.head.commit


If current_head is None here, should we return False early or should we proceed to try to check out the specific commit by the user thinking . The Exceptions for checking if digest exists or whether we can checkout digest (after here) should be caught and return False.

What I meant is that in this line, there might be the case where current_head is None (there might be some errors with extracting the head commit). Should we exit on error early here, or we ignore it to at least try to check out the specific digest down below (where any exception would be caught):

if commit_exists(git_obj, digest): # The digest was found in the history of the branch. try: git_obj.checkout(digest) except GitCommandError as error: logger.error( "Commit %s cannot be checked out. Error: %s", digest, error, ) return False else: logger.info("Cannot find commit %s on branch %s. Please check the configuration.", digest, res_branch) return False

It should be safe to try again to check out the digest if it exists, because commit_exists should return gracefully if commit doesn't exist.

…ath (#125) Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Mar 28, 2023

tromai linked an issue Mar 28, 2023 that may be closed by this pull request

Macaron pull the latest when analyzing a local cloned repository #117

Closed

tromai marked this pull request as ready for review March 28, 2023 05:53

tromai requested a review from behnazh-w as a code owner March 28, 2023 05:53

tromai self-assigned this Mar 28, 2023

tromai changed the title ~~fix: do not pull the latest when analyzing local repo~~ fix: do not pull the latest when analyzing a target with local repo path Mar 28, 2023

tromai added 4 commits March 31, 2023 11:32

fix: do not pull the latest when analyzing a target with a local repo…

da02393

… path Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>

chore: add tests for analyzing local repo path

cd4ccd7

Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>

chore: add git config before committing in the test repo

e959dbd

Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>

chore: run the git config user identity command in the repo directory

f04ed59

Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>

tromai force-pushed the 117-macaron-pull-the-latest-when-analyzing-a-local-cloned-repository branch from ade50b6 to f04ed59 Compare March 31, 2023 01:33

behnazh-w reviewed Apr 18, 2023

View reviewed changes

chore: address PR comments

dc986c6

Add checks for the final HEAD commit after checking out the repository for edge cases. Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>

behnazh-w reviewed Apr 18, 2023

View reviewed changes

chore: fix conditions for pulling newest changes

e1e76d7

Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>

tromai commented Apr 19, 2023

View reviewed changes

behnazh-w approved these changes Apr 19, 2023

View reviewed changes

tromai merged commit 9778e39 into staging Apr 19, 2023

tromai deleted the 117-macaron-pull-the-latest-when-analyzing-a-local-cloned-repository branch April 19, 2023 04:35

tromai mentioned this pull request Apr 19, 2023

feat: support running the analysis with SBOM and the main software component with no repository #165

Merged

art1f1c3R pushed a commit that referenced this pull request Nov 29, 2024

fix: do not pull the latest when analyzing a target with local repo p…

5fa7759

…ath (#125) Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>

	echo "apache/maven: test not doing a git pull when analyzing a local cloned repo."
	echo "apache/maven: test not pulling from remote for a locally cloned repo."

		if not offline_mode:
		if not digest or (digest and not commit_exists(git_obj, digest)):

	if not offline_mode:
	if not digest or (digest and not commit_exists(git_obj, digest)):
	if not offline_mode and (not digest or not commit_exists(git_obj, digest):

	If ``offline_mode`` is True. This method will not perform any pulling before checking out the branch
	If ``offline_mode`` is set, this method will not pull from remote while checking out the branch or commit.

fix: do not pull the latest when analyzing a target with local repo path #125

fix: do not pull the latest when analyzing a target with local repo path #125

Uh oh!

Conversation

tromai commented Mar 28, 2023

Uh oh!

tromai commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tromai commented Mar 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tromai Apr 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tromai Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tromai commented Mar 28, 2023 •

edited

Loading

tromai Apr 18, 2023 •

edited

Loading

tromai Apr 19, 2023 •

edited

Loading