feat: add blueprints command #1244
Conversation
| } | ||
|
|
||
| fmt.Fprintf(opts.out, "Loaded blueprint: %s\n", bp.Name) | ||
| if bp.Title != "" { |
There was a problem hiding this comment.
I think you can roll all this fmt.Fprintf to be part of bp.String()
fmt.Fprintln(opts.out, bp.String())
|
This code looks nice and self contained, so what is missing for me is the motivation for why this lives in src-cli since it could just as easily live in its own self contained world. I will try motivate it, but interested if this is useful motivation:
The 2nd point isn't that big of a deal, since you could just shell out to src-cli to do your graphql queries. Additionally src-cli has very naive auth at the moment (although we do want to improve that), so it doesn't provide much over just setting a header on a curl request. The 1st point I guess is the main motivation? |
| // FindBlueprints recursively searches rootDir for blueprint.yaml files and | ||
| // returns the successfully loaded blueprints. Invalid or malformed blueprints | ||
| // are silently skipped to allow discovery to continue. | ||
| func FindBlueprints(rootDir string) ([]*Blueprint, error) { |
There was a problem hiding this comment.
I think this should be just Find, because we're already in the blueprint package, otherwise we repeat ourselves with blueprint.FindBlueprints
There was a problem hiding this comment.
Good call, I was waffling back and forth on naming for a number of things in this package.
| ) | ||
|
|
||
| // BlueprintSource provides access to blueprint files from various sources. | ||
| type BlueprintSource interface { |
There was a problem hiding this comment.
Maybe just Source since outside of the package it will be blueprint.Source ?
| var src blueprint.BlueprintSource | ||
| var err error | ||
|
|
||
| if opts.subdir == "" { |
There was a problem hiding this comment.
nit: maybe this distinction should be made in a top level blueprint.NewSource / blueprint.ResolveSource method? We then unexport ResolveRootSource / ResolveBlueprintSource and just have a single Resolve method exposed that returns the correct source for us.
| } | ||
|
|
||
| if len(found) == 0 { | ||
| fmt.Fprintf(opts.out, "No blueprints found in repository\n") |
There was a problem hiding this comment.
| fmt.Fprintf(opts.out, "No blueprints found in repository\n") | |
| fmt.Fprintf(opts.out, "No blueprints found in %s\n", rootDir) |
| return nil | ||
| } | ||
|
|
||
| fmt.Fprintf(opts.out, "Found %d blueprint(s) in repository\n\n", len(found)) |
There was a problem hiding this comment.
| fmt.Fprintf(opts.out, "Found %d blueprint(s) in repository\n\n", len(found)) | |
| fmt.Fprintf(opts.out, "Found %d blueprint(s) in repository\n", len(found)) |
unless the extra \n was intentional?
| summary := &ExecutionSummary{} | ||
|
|
||
| if len(bp.Dashboards) > 0 { | ||
| fmt.Fprintf(e.out, "\nExecuting dashboard mutations...\n") |
There was a problem hiding this comment.
nit: I'd rather make this method not print anything. It's whomever calls this job to print the result. I can understand the need to see "as it executes" but ultimately this can be done after this method executes.
for _, r := range summary.Resources[KindDashboard] { ... }|
|
||
| // ExecutionSummary contains the results of executing all resources in a blueprint. | ||
| type ExecutionSummary struct { | ||
| Resources []ResourceResult |
There was a problem hiding this comment.
I think it would be nicer to have Resources map[ResourceKind]ResourceResult
| } | ||
|
|
||
| // PrintExecutionSummary writes a human-readable summary of execution results. | ||
| func PrintExecutionSummary(out io.Writer, s *ExecutionSummary, dryRun bool) { |
There was a problem hiding this comment.
nit: think this should be func (s *ExecutionSummary) String() string
| // Prepare makes the blueprint directory available and returns its path. | ||
| // The cleanup function MUST be called when done (may be nil for sources | ||
| // that don't require cleanup). | ||
| Prepare(ctx context.Context) (blueprintDir string, cleanup func() error, err error) |
There was a problem hiding this comment.
nit: Would a better name be Init or Setup ?
| } | ||
|
|
||
| blueprintDir, cleanup, err := src.Prepare(ctx) | ||
| if cleanup != nil { |
There was a problem hiding this comment.
why not always return a valid cleanup? then you don't have to check it the whole time and can just do defer cleanup() with confidence
The big reason for now is to add it in something customers most likely already have approved in their environments, even if it isn't a publicly documented command. (There's a few other examples of these in src-cli). Its usually easier to just have a customer use src-cli rather that struggle with waiting for security approval for ANOTHER tool. Even if src-cli requires approval for an update, in my experience, that's usually faster than a whole new tool. There's another thread re: maintenance/inclusion of pre-built dashboards in-product, but having this is src-cli is a quick fix while that is hashed out. |
|
Great, security approval is indeed a great reason to bundle things into this tool. That helps with my thinking for other things we could be doing, like src-expose, etc. |
|
I haven't looked deeply at the code yet, but my main high level feedback is I'm surprised there is so much code given this seems to mostly just run graphql queries from a yaml file. However, it is self contained and tested so not the best reason to block this. But would be interesting to know if there is a simpler design here which results in far less code and maybe also makes it easier to use. |
|
@keegancsmith I'm going back through it to refactor a bit. A lot of the logic is for the yaml metadata parsing of the blueprint spec I created when I started working on this idea. The other primary driver here is to package everything up and make it super easy for a user to pull things into an instance without actually having to know GQL (leaving that to our team). Yes, this can just be a cURL command, but, as a customer, if I can take a one-liner command with a known tool and just load a bunch of relevant functionality into an instance, I'm going to want to do that, rather than fuss with making a script to do it, or learning GQL when I'm just getting started with Sourcegraph. Long-term, the correct move is to move popular/common examples/use-cases in-product, which we've been discussing elsewhere, but for now, this is an easy way for our team to explore options further and make it really easy to distribute solutions to customers. |
|
I'll leave this in draft and ping you and @burmudar once I've done a bit more refactoring. |
Test plan
All implemented blueprint sub-commands have been tested locally. This is intended to remain undocumented/internal only for now. Mainly intended for the FE team to work with customers to quickly import pre-built solutions.