Basic Cirrus CLI with validate command #1
Conversation
There was a problem hiding this comment.
Let's also change the file structure since we are planning to open source Cirrus agent in the repo as well. So let's put all the CLI related stuff in cli folder and all the agent stuff in agent folder. All the common sources we can leave in pkg folder.
How about mocing:
cmd/cirrus/main.gotocli/cmd.gointernal/cmd/root.gotocli/internal/root.gointernal/cmd/validate.gotocli/internal/commands/validate.gointernal/cmd/validate_test.gotocli/internal/commands/validate_test.go
| task: | ||
| container: | ||
| image: golangci/golangci-lint:latest | ||
| name: Lint |
There was a problem hiding this comment.
Let's add the annotations here
| ErrTransport = errors.New("transport error") | ||
| ) | ||
|
|
||
| type Parser struct { |
There was a problem hiding this comment.
I'd call it GraphQLValidator since it's not actually parsing. The upcoming GRPC endpoint will be parsing indeed.
There was a problem hiding this comment.
But it still does some parsing behind the scenes, right? Otherwise it'd be impossible to fill the Errors field in the Result struct.
The idea here is to abstract away GraphQL-related bits as much as possible since we already know that in the near future it will be superseded by a gRPC endpoint.
This allows us to keep the API intact while it undergoes internal changes, except for the GraphqlEndpoint field, but the only reason an API consumer might want to set this field is tests.
You still sure it'd make a difference?
There was a problem hiding this comment.
Sure, with replacing it with GRPC I don't mind to live it as Parser here.
pkg/parser/parser_test.go
Outdated
| } | ||
|
|
||
| func absolutize(file string) string { | ||
| return filepath.Join("..", "..", "test", "fixtures", "parser", file) |
There was a problem hiding this comment.
Let's maybe move the files to testdata folder? Saw it here that it's a convetion.
This reverts commit 7d5886a.
This would cause the See https://tip.golang.org/cmd/go/#hdr-Compile_packages_and_dependencies:
Also, the
See ad7efdc. |
|
Should we then put the agent in a separate repo as well? I was debating about that this weekend 🤔 I'm a fan of having everything in one repo, but I also see the point that the agent will not change as frequently, plus having it in separate repositories will force us to make backward compatible changes which is good for Cirrus CI as a distributed executor. |
| ErrTransport = errors.New("transport error") | ||
| ) | ||
|
|
||
| type Parser struct { |
There was a problem hiding this comment.
Sure, with replacing it with GRPC I don't mind to live it as Parser here.
I don't see how the agent's slow rate of change makes it a bad target for inclusion. Does it have something to do with compilation times? How about we keep the agent where it is right now, and as we progress towards a less "thin" CLI we'll see the overlap more clearly, and adapt accordingly? |
|
Let's discuss offline today. 🤔 |
Thought these files might be useful for other packages in the future, but the way you described it makes more sense now, see 759b054. |
Let's roll.