Skip to content

Conversation

@LawnGnome
Copy link
Contributor

@LawnGnome LawnGnome commented Aug 8, 2020

This PR implements the new campaign spec workflow, most notably src campaigns apply. Therefore, it will close sourcegraph/sourcegraph#12333, close sourcegraph/sourcegraph#11350, and close #259.

Note that this PR includes #268, but that should be reviewed separately.

Here's a GIF:

out

This gives us some basic primitives to output structured, human readable
information from commands. This is currently intended only for the campaigns
work taking place in #260, but also gives us options for additional
functionality elsewhere, such as internal/api.

This is somewhat duplicative with things that we currently have in cmd/src and
internal/campaigns; I intend to gradually clean those up as time permits.
@LawnGnome LawnGnome force-pushed the aharvey/campaign-apply-prototype branch from 8f2eaa5 to 9ac65fb Compare August 20, 2020 06:00
@LawnGnome LawnGnome requested a review from mrnugget August 20, 2020 06:01
@LawnGnome LawnGnome marked this pull request as ready for review August 20, 2020 06:02
@LawnGnome LawnGnome marked this pull request as draft August 20, 2020 06:08
@LawnGnome LawnGnome marked this pull request as ready for review August 20, 2020 06:26
Copy link
Contributor

@mrnugget mrnugget left a comment

Choose a reason for hiding this comment

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

👏 This is really well architected, thank you! Good eye for how to slice up and refactor existing code 🙂

I bet output was fun to write 😄

One request for this iteration: I'd appreciate some documentation (in sourcegraph/sourcegraph I think) that describes the semantics behind campaigns apply: how the list of repositories is generated, how the shell command is run in the container, etc.

@mrnugget
Copy link
Contributor

Created a ticket for something I noticed while trying out this branch: https://github.com/sourcegraph/sourcegraph/issues/13172 I don't think it needs to be a part of this PR but we (and also not necessarily you, Adam) need to tackle this in this iteration.

@mrnugget
Copy link
Contributor

Before merging: run go mod tidy in this branch

LawnGnome and others added 7 commits August 20, 2020 09:32
Co-authored-by: Thorsten Ball <mrnugget@gmail.com>
Co-authored-by: Thorsten Ball <mrnugget@gmail.com>
Co-authored-by: Thorsten Ball <mrnugget@gmail.com>
Co-authored-by: Thorsten Ball <mrnugget@gmail.com>
Co-authored-by: Thorsten Ball <mrnugget@gmail.com>
Co-authored-by: Thorsten Ball <mrnugget@gmail.com>
LawnGnome and others added 5 commits August 20, 2020 09:59
Co-authored-by: Thorsten Ball <mrnugget@gmail.com>
Except that implies a certain level of skill and expertise that isn't evident
here, so this is more
word-banging-on-hot-metal-and-trying-not-to-set-the-workshop-on-fire.
@LawnGnome LawnGnome merged commit 9116e61 into master Aug 20, 2020
Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

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

Great work all the way through :) I really appreciate the service layer, the simplicity to understand the run logic and the output package 👍

scjohns pushed a commit that referenced this pull request Apr 24, 2023
Co-authored-by: Thorsten Ball <mrnugget@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request team/code-search

Projects

None yet

Development

Successfully merging this pull request may close these issues.

src campaigns: unknown subcommand "apply" src-cli: Implement new campaigns workflow Add CLI option to update a campaign

4 participants