Skip to content

Comments

[PLUTO-1412] test init#113

Merged
zhamborova merged 15 commits intomainfrom
PLUTO-1412_test_init_no_token
May 14, 2025
Merged

[PLUTO-1412] test init#113
zhamborova merged 15 commits intomainfrom
PLUTO-1412_test_init_no_token

Conversation

@zhamborova
Copy link
Contributor

No description provided.

@codacy-production
Copy link

codacy-production bot commented May 13, 2025

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
-5.81% 65.79%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (c584641) 4240 1568 36.98%
Head commit (24cb209) 4261 (+21) 1328 (-240) 31.17% (-5.81%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#113) 38 25 65.79%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

@zhamborova zhamborova force-pushed the PLUTO-1412_test_init_no_token branch 3 times, most recently from 9259d2f to 51e375c Compare May 13, 2025 16:23
@zhamborova zhamborova force-pushed the PLUTO-1412_test_init_no_token branch from e9739d6 to 8a5fefb Compare May 13, 2025 18:22
Comment on lines -104 to -105
// Override the function to find rules.yaml to use our test file
originalRulesFilePath := filepath.Join("plugins", "tools", "semgrep", "rules.yaml")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this change is related to the fact that semgrep config could not be created in subdirs with init cuz it could not find rules.yaml before

@zhamborova zhamborova marked this pull request as ready for review May 14, 2025 11:00
// Get the default rules file location
rulesFile := filepath.Join("plugins", "tools", "semgrep", "rules.yaml")
// Get the executable's directory
execPath, err := getExecutablePath()
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't love us messing with executable path. Don't we have a workdir or something that we can add and have on the config?

Maybe the problem here is more related to something that is not correctly embeded? I think I saw some change on some PR of @andrzej-janczak that he actually needed to update a Join that would not work on windows, and needed to make '/'. Maybe he has any input here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, we can test but should work theoretically

filepath.Join is specifically designed to be cross-platform:
On Windows, it uses backslashes ()
On Unix-like systems, it uses forward slashes (/)
It handles the path separator differences automatically
os.Executable() is also cross-platform:
Returns the correct path format for the OS
On Windows, it returns paths with backslashes
On Unix, it returns paths with forward slashes
os.Stat and os.ReadFile are also cross-platform and handle path separators correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

we tested this on both Windows native and WSL and both are working ok

Copy link
Contributor

@machadoit machadoit left a comment

Choose a reason for hiding this comment

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

Pretty cool!

@zhamborova zhamborova merged commit 796ade5 into main May 14, 2025
10 checks passed
@alerizzo alerizzo deleted the PLUTO-1412_test_init_no_token branch June 3, 2025 09:44
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.

3 participants