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

use simple path for cloning in adhoc flow #737

Merged
merged 5 commits into from
Apr 5, 2024
Merged

use simple path for cloning in adhoc flow #737

merged 5 commits into from
Apr 5, 2024

Conversation

smonero
Copy link
Contributor

@smonero smonero commented Mar 25, 2024

we don't need to use the uuid and all that stuff since we only have 1 repo on our batch pod. To actually use this func, in the other PR we will pass in the option of simplePath = true

@smonero smonero changed the title ok use simple path for cloning in adhoc flow Mar 28, 2024
Copy link
Contributor

@miguelmolina95 miguelmolina95 left a comment

Choose a reason for hiding this comment

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

do we want to add a test to ensure path is what we expect?

@@ -112,6 +119,10 @@ func (g *RepoFetcher) generateDirPath(repoName string) string {
return filepath.Join(g.DataDir, workingDirPrefix, repoName, uuid.New().String())
}

func (g *RepoFetcher) GenerateSimpleDirPath(repoName string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend against exposing functions publicly when it can be avoided

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To test it wouldn't we have to put it in the same package then? Since they are in separate packages I wasn't sure how to make a test unless its exposed

Copy link
Contributor

Choose a reason for hiding this comment

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

You can create a unit test against running Fetch with the new optional field you've created and validate that the generated path matches what you expected.

@smonero smonero merged commit bcf01ec into main Apr 5, 2024
3 checks passed
@smonero smonero deleted the simplePath1 branch April 5, 2024 19:19
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