-
Notifications
You must be signed in to change notification settings - Fork 70
Add src actions exec command to generate arbitrary campaign diffs
#80
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
Conversation
|
I'm picking this up in 3.13 |
22465a1 to
baa7686
Compare
mrnugget
left a comment
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.
Leaving a first batch of notes to myself while reviewing/working on this PR.
cmd/src/actions_exec.go
Outdated
| } | ||
| displayUserCacheDir := strings.Replace(userCacheDir, os.Getenv("HOME"), "$HOME", 1) | ||
| var ( | ||
| fileFlag = flagSet.String("f", "<stdin>", `The action file. (required)`) |
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.
Note to myself: I think it might be more idiomatic to make -f optional and read from stdin if it's not set. Or use -f - to say "read from stdin"
mrnugget
left a comment
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.
Left more notes to myself
|
|
||
| // Add to cache if successful. | ||
| if err == nil { | ||
| if err := x.opt.cache.set(ctx, cacheKey, status.Patch); err != nil { |
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.
Note to myself: does this also write to the cache when status.Patch == CampaignPlanPatch{}? Or do we need the len(patch) > 0 check here too?
I think this makes a lot more sense from a users perspective, especially with the upcoming changes in #80.
I think this makes a lot more sense from a users perspective, especially with the upcoming changes in #80.
|
Note: I think the problem is not |
|
Update: I think I'm happy with the code so far and would move on to giving a last shot at fixing the moving/scrolling UI, documentation and adding |
The corresponding Sourcegraph GraphQL API mutations will be (temporarily) removed until they support full remote execution of arbitrary tools.
|
I broke the UI logic, I think, since it now also prints progress when the output is piped to another command. |
|
Documentation: https://github.com/sourcegraph/sourcegraph/pull/8064 |
src actions exec command to generate arbitrary campaign diffssrc actions exec command to generate arbitrary campaign diffs
|
I'm going to merge this now because the basic feature set and documentation is there. It works, but can be polished:
But the PR has gotten quite big and further additions would make it unwieldy. Once it's merged I'll open new PRs to polish things. @sqs Let me know if you have any specific concerns besides the ones I mentioned, or whether your don't think the things I've mentioned don't have priority right now. |
I think this makes a lot more sense from a users perspective, especially with the upcoming changes in #80.
) * WIP: add `src actions exec` command to generate arbitrary campaign diffs * Use `/tmp` as TempDir prefix because it works with Docker for Mac * Remove fixed TODO comment * Fall back to degraded behavior for GNU diff <3.3 * Remove example actions * Check all diff flags that we need * Various small fixes and cleanups * Check whether stdin is a pipe before attempting to read * Extract getDockerImageContentDigest * Extract Actions UI printer into separate struct * Log if skipping repo because of missing default branch * Stop leaking goroutine * Move preparation of action to separate function * Flush at the end of update not at the beginning * Add timeout flag for action execution * We don't need a trailing slash * Consistently use temp dir prefix * Rename types to get rid of redundant information * Use "-" to signify STDIN * Remove campaign plan creation via spec The corresponding Sourcegraph GraphQL API mutations will be (temporarily) removed until they support full remote execution of arbitrary tools. * Revert extraction of UI printer to fix piping * Add `campaigns create` command * Add documentation examples for `src actions exec` * Print friendly message when a campaign has been created * More documentation * Use full or relative URL of Campaign * Use ResolveReference to construct Campaign URL Co-authored-by: Thorsten Ball <mrnugget@gmail.com>
Usage:
Run:
This will create a campaign that adds GitHub Action LSIF build steps to repositories matched in the action.json file (see and change the query to match repositories that exist on your instance). Note: You should only use this with the test sd9 and sd9org repositories, because if you click the link and press Create, it will create PRs on real live repositories!
Ping me (@sqs) for help on anything here if you pick up this PR.
TODOs:
diffCLI flags we use only work on GNU diff not BSD diff, and the Docker volume mounting breaks due to macOS's/var/tmp/folders/...sandboxing)Document(documentation & guide here: https://github.com/sourcegraph/sourcegraph/pull/8064 and also in the-helpoutput of the commands)src campaigns createcommand is not implementedactions/dir. These are good candidates for examples in the docs, but they don't belong in this repo.