-
-
Notifications
You must be signed in to change notification settings - Fork 952
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
Performance enhancement of GitTopLevelDir in shell pkg #2668
base: master
Are you sure you want to change the base?
Performance enhancement of GitTopLevelDir in shell pkg #2668
Conversation
…ng and then reused + added simple benchmark test
…ncement/git-top-level-cache
@@ -53,3 +53,13 @@ func TestRunShellOutputToStderrAndStdout(t *testing.T) { | |||
assert.True(t, strings.Contains(stderr.String(), "Terraform"), "Output directed to stderr") | |||
assert.True(t, len(stdout.String()) == 0, "No output to stdout") | |||
} | |||
|
|||
func BenchmarkPerformanceOfGitTopLevelDir(b *testing.B) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, will be helpful to have a functional test that will validate cache usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, I'll add it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I have added a test 👍
shell/run_shell_cmd.go
Outdated
@@ -230,23 +230,29 @@ type CmdOutput struct { | |||
Stderr string | |||
} | |||
|
|||
var gitTopLevelDir string = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this approach will work in case of multiple git repositories
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'll consider this
…onfig for new generic cache + added tests for generic cache as well as for the GitTopLevelDir function that uses the cache
@denis256 Okay, so I have added a test for the caching. Previously, the StringCache, etc. were all in the config package. The problem with that is, that I cannot use them in the shell package due to circular import. So I have moved out all of the caches except the Also, I have made the cache structs generic with type constraint since the implementation of the This approach also supports multiple git repos now. I have also updated the gist with the new test output and new benchmark. The benchmark is almost identical to the previous one. |
Hi. Any updates on merging this PR? Thanks |
Hi,
|
@denis256 I've updated the branch and fixed the conflict |
This PR still misses integration tests( |
Description
Fixes #2344.
git rev-parse --show-toplevel
at the beginning and then re-use the previously found git root pathTODOs
Read the Gruntwork contribution guidelines.
Update the docs.
I didn't update the docs since I am not changing any function signatures or behavior, I am just making the specified code more efficient. If I missed some document where my change chould be described or added to, please tell me.
Run the relevant tests successfully, including pre-commit checks.
Include release notes. If this PR is backward incompatible, include a migration guide.
Release Notes (draft)
GitTopLevelDir will only run
git rev-parse --show-toplevel
at the beginning and then re-use the previously found git root pathAdded / Removed / Updated [X].
Gist
Gist: https://gist.github.com/RaphSku/8cb9aae84ea374de85d621b6cc6bf8f5
for the test output and benchmark