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.

Sign up for GitHub

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

Jump to bottom

feat(foreach): run against previously failed or successful repos #147

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat(foreach): run against previously failed or successful repos #147

wants to merge 1 commit into from

Conversation

Dan7-7-7
Copy link
Collaborator

@Dan7-7-7 Dan7-7-7 commented Oct 7, 2024

sledigabel
sledigabel reviewed Oct 7, 2024
View reviewed changes
cmd/foreach/foreach.go

failedResultsDirectory string
failedReposFileName string
failedReposSymlink = ".latest_failed"
Copy link
Contributor

Choose a reason for hiding this comment

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

those could be const?

sledigabel
sledigabel reviewed Oct 7, 2024
View reviewed changes
cmd/foreach/foreach.go
@@ -65,7 +80,9 @@ marks that no further options should be interpreted by turbolift.`,
Args: cobra.MinimumNArgs(1),
}

cmd.Flags().StringVar(&repoFile, "repos", "repos.txt", "A file containing a list of repositories to clone.")
cmd.Flags().StringVar(&repoFile, "repos", "", "A file containing a list of repositories to clone.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is needed - this mutex between repos.txt and successful/failed seems complicated, should we just ignore the repoFile if --successful or --failed is set?

sledigabel
sledigabel reviewed Oct 7, 2024
View reviewed changes
cmd/foreach/foreach.go
}
err := os.Symlink(successfulReposFileName, successfulReposSymlink)
if err != nil {
logger.Warnf("Failed to create symlink for successful repos: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

warn or fail?
Could there be a reason why this should fail?

rnorth
rnorth reviewed Oct 7, 2024
View reviewed changes
cmd/foreach/foreach.go

overallResultsDirectory string

successfulResultsDirectory string
successfulReposFileName string
successfulReposSymlink = ".latest_successful"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we definitely need two symlinks? If we have our output directories to be something like:

some_temp_dir_from_os/
    some_random_bit/
        successful/
            repos.txt
            logs/
        failed/
            repos.txt
            logs/

Then we could just use one symlink, to the /some_temp_dir_from_os/some_random_bit path and infer the rest as needed.

sledigabel
sledigabel reviewed Oct 7, 2024
View reviewed changes
cmd/foreach/foreach.go
@@ -151,6 +185,27 @@ func setupOutputFiles(campaignName string, command string) {
defer successfulReposFile.Close()
defer failedReposFile.Close()

if _, err := os.Lstat(successfulReposSymlink); err == nil {
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 prob just override it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rnorth rnorth rnorth left review comments

@sledigabel sledigabel sledigabel left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Dan7-7-7 @rnorth @sledigabel