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

Fixed wrong root identification in module detection #179

Conversation

eranturgeman
Copy link
Contributor

@eranturgeman eranturgeman commented Sep 15, 2024

  • The pull request is targeting the dev branch.
  • The code has been validated to compile successfully by running go vet ./....
  • The code has been formatted properly using go fmt ./....
  • All static analysis checks passed.
  • All tests have passed. If this feature is not already covered by the tests, new tests have been added.
  • All changes are detailed at the description. if not already covered at JFrog Documentation, new documentation have been added.

In getExistingRootDir we find the highest common ancestor for every descriptor, and refer this descriptor to this common ancestor rather then its original working dir.
As part of the process we check under some condition, if the current wd is a prefix to the current root, and if so - wd is the new root.
We encountered a wrong identification in the following scenario: if wd is indeed a prefix to root but not a path prefix, we wrongfully take wd as the new root.
Example:
root = "dir1/dir2"
wd = "dir"

wd is indeed a prefix to root (by characters) but is not actually a path prefix since the path prefix to root is "dir1" and not "dir"

This PR adds fix for this issue so we identify if wd is a path prefix to the root

Also, this PR added a test case to TestGetTechInformationFromWorkingDir, that have failed due to the build, and is now added after the fix

@eranturgeman eranturgeman added bug Something isn't working safe to test Approve running integration tests on a pull request labels Sep 15, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Sep 15, 2024
@eranturgeman eranturgeman changed the title Fixed wrong root identification in getExistingRootDir the checked wd is prefix to root but not a path prefix Fixed wrong root identification in module detection Sep 15, 2024
@eranturgeman eranturgeman added the safe to test Approve running integration tests on a pull request label Sep 15, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Sep 15, 2024
Copy link
Contributor

@attiasas attiasas left a comment

Choose a reason for hiding this comment

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

LGTM

utils/techutils/techutils.go Outdated Show resolved Hide resolved
…o fix-getExistingRootDir-incorrect-root-identification
@eranturgeman eranturgeman added the safe to test Approve running integration tests on a pull request label Sep 15, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Sep 15, 2024
@eranturgeman eranturgeman added the safe to test Approve running integration tests on a pull request label Sep 15, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Sep 15, 2024
@eranturgeman eranturgeman added the safe to test Approve running integration tests on a pull request label Sep 16, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Sep 16, 2024
@eranturgeman eranturgeman added the safe to test Approve running integration tests on a pull request label Sep 16, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Sep 16, 2024
Copy link

👍 Frogbot scanned this pull request and did not find any new security issues.


@eranturgeman eranturgeman merged commit c2d83db into jfrog:dev Sep 16, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants