[CIRCLE-12186] optionally read config file from stdin #23

johnswanson · 2018-07-16T23:04:11Z

No description provided.

codecov-io · 2018-07-16T23:05:06Z

Codecov Report

Merging #23 into master will decrease coverage by 0.92%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master      #23      +/-   ##
==========================================
- Coverage   41.75%   40.82%   -0.93%     
==========================================
  Files          11       11              
  Lines         685      703      +18     
==========================================
+ Hits          286      287       +1     
- Misses        376      393      +17     
  Partials       23       23

Impacted Files	Coverage Δ
cmd/root.go	`74.5% <ø> (-0.5%)`	⬇️
cmd/orb.go	`24.59% <33.33%> (-1.43%)`	⬇️
cmd/config.go	`43.1% <33.33%> (-3.84%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d28439...f70fabd. Read the comment docs.

zzak · 2018-07-17T05:18:35Z

cmd/orb.go

@@ -41,10 +41,10 @@ func newOrbCommand() *cobra.Command {

 	orbCommand.AddCommand(orbListCommand)

-	orbValidateCommand.PersistentFlags().StringVarP(&orbPath, "path", "p", "orb.yml", "path to orb file")
+	orbValidateCommand.PersistentFlags().StringVarP(&orbPath, "path", "p", "orb.yml", "path to orb file ('-' for STDIN)")


Having to type -p when I just want to pipe some file as stdin to a program is painful.

For certain flags, which aren't optional (behavior changing) such as the path to your config, a truly vital option for this command (or we default) shouldn't be a flag IMO.

I'd like if we could just type circleci orb validate path/to/orb.yml or leave path/to/orb.yml out entirely for the same results (if it were default) and in this use-case circleci orb validate -. But I may just be dreaming. 🤔

+1 - my understanding is we added path as a flag because we would have a default value of the path, but we should probably kill the default value, have path be required, pass it without a flag. I think we talked about a pattern where we'd try to limit required arguments and let you pass them without a flag. But if that's a major effort we should discuss its relative value.

Let's not make the config path be required. The current CLI tool has a command circleci config validate and we don't want to break our customer's workflows.

zzak

I've some UX concerns with this change, but agree we should allow stdin for these commands.

Would like to get a test for this so if we change it, we'll be able to validate our code still works.

johnswanson · 2018-07-17T23:25:37Z

@zzak : paired with @hannahhenderson on this today, made configPath and orbPath positional arguments (configPath is optional and defaults to .circleci/config.yml while orbPath is required).

Testing here feels super awkward to me, mostly because I am not very familiar with Go. I basically copied and pasted another test, just changing the way the command is executed slightly, to pass the orb content in as stdin instead of as a path. Is there a better way?

eric-hu

I cloned this branch and tried interacting with it. Left a couple in line notes. Additionally:

1 (a). I'm feeling iffy about removing a default value. Compare these (before):
circleci orb validate (implicit .circleci/config.yml)
circleci orb validate --path myorb/config.yml
to these (after):
circleci orb validate myorb/config.yml
cat myorb/config.yml | circleci orb validate -

The change to use an argument instead of a flag makes sense. I think it'd be nicer if we didn't drop support for a default path, though.

1 (b). Assuming we are okay with dropping support for a default path, when 0 args are received, the feedback message is cryptic. Perhaps it should be the same as --help?

$ go run main.go orb expand
Error: accepts 1 arg(s), received 0
exit status 255

vs

$ go run main.go orb expand --help
expand an orb.yml

Usage:
  circleci orb expand [flags]

Flags:
  -h, --help   help for expand

Global Flags:
  -e, --endpoint string   the endpoint of your CircleCI GraphQL API (default "https://circleci.com/graphql-unstable")
  -t, --token string      your token for using CircleCI
  -v, --verbose           Enable verbose logging.

Regarding copying and pasting tests, I think that's totally fine. I've been doing that, and we'll figure out better abstractions and refactorings as we Go. I don't think any of us are expert Gophers yet.

eric-hu · 2018-07-18T17:32:37Z

cmd/orb_test.go

+				stdin, err := command.StdinPipe()
+				Expect(err).ToNot(HaveOccurred())
+				go func() {
+					defer stdin.Close()


eric-hu · 2018-07-18T17:33:15Z

cmd/orb_test.go

+
+				Expect(err).ShouldNot(HaveOccurred())
+				// the .* is because the full path with temp dir is printed
+				Eventually(session.Out).Should(gbytes.Say("Orb at - is valid"))


The feedback message from piping in is awkward. Maybe this should be something like orb passed from stdin is valid instead?

$ cat ./.circleci/config.yml | go run main.go orb validate - Orb at - is valid

zzak · 2018-07-23T03:15:26Z

ca96a7f is a quick refactor to clean up the cruft of building an expect json response that matches the formatting given from api package.

- configPath has a default value of .circleci/config.yml - orbPath is required

Also changes config commands to use a positional arg max of 1, and include it in usage documentation for `--help`. Ensure temp files and dirs are removed Also adds a test for testing orb validate with default path

``` panic: runtime error: index out of range goroutine 1 [running]: github.com/CircleCI-Public/circleci-cli/cmd.validateOrb(0xc420183680, 0xb4aac8, 0x0, 0x0, 0x0, 0x0) /home/zzak/work/go/src/github.com/CircleCI-Public/circleci-cli/cmd/orb.go:137 +0x1a0 ```

Adding files into the working directory of an executable built by gomega is too hard, they create a temporary directory for each time and compile the binary to there each time you instruct it to. So creating a situation where a file exists at the current $PWD when executing the test binary, e.g. `.circleci/config.yml` would require you to create that file inside the temp directory owned by gomega.

By allowing us to specify the path when creating a temp file, we can write directly to the working directory of the test executable. This resolves my comments on 74deab2. Also make sure temp files and folders are properly disposed of, this is something we're leaving up to who opened them first. Since using `defer` properly won't work because context is left between tests -- we should use `AfterEach` to close them ourselves.

zzak · 2018-07-24T09:21:31Z

We can now support optionally a positional arg to specify the input file, even as stdin with "-", or fall back to the default.

This includes the following commands:

config expand
config validate
orb expand
orb publish
orb validate

config expand

/tmp/tmp.7fGNduimQt => ./circleci help config expand
Expand the config.

Usage:
  circleci config expand [config.yml] [flags]

Flags:
  -h, --help   help for expand

Global Flags:
  -e, --endpoint string   the endpoint of your CircleCI GraphQL API (default "https://circleci.com/graphql-unstable")
  -t, --token string      your token for using CircleCI
  -v, --verbose           Enable verbose logging.
/tmp/tmp.7fGNduimQt => ./circleci config expand
Error: required key [version] not found
/tmp/tmp.7fGNduimQt => cat .circleci/config.yml 
{}

config validate

/tmp/tmp.7fGNduimQt => ./circleci help config validate
Check that the config file is well formed.

Usage:
  circleci config validate [config.yml] [flags]

Aliases:
  validate, check

Flags:
  -h, --help   help for validate

Global Flags:
  -e, --endpoint string   the endpoint of your CircleCI GraphQL API (default "https://circleci.com/graphql-unstable")
  -t, --token string      your token for using CircleCI
  -v, --verbose           Enable verbose logging.
/tmp/tmp.7fGNduimQt => ./circleci config validate
Error: required key [version] not found
/tmp/tmp.7fGNduimQt => ./circleci config validate .circleci/config.yml 
Error: required key [version] not found
/tmp/tmp.7fGNduimQt => cat .circleci/config.yml | ./circleci config validate -
Error: required key [version] not found

orb expand

/tmp/tmp.7fGNduimQt => ./circleci help orb expand
expand an orb.yml

Usage:
  circleci orb expand [orb.yml] [flags]

Flags:
  -h, --help   help for expand

Global Flags:
  -e, --endpoint string   the endpoint of your CircleCI GraphQL API (default "https://circleci.com/graphql-unstable")
  -t, --token string      your token for using CircleCI
  -v, --verbose           Enable verbose logging.

orb publish

/tmp/tmp.7fGNduimQt => ./circleci help orb publish
publish a version of an orb

Usage:
  circleci orb publish [orb.yml] [flags]

Flags:
  -h, --help                 help for publish
  -i, --orb-id string        id of orb to publish
  -o, --orb-version string   version of orb to publish

Global Flags:
  -e, --endpoint string   the endpoint of your CircleCI GraphQL API (default "https://circleci.com/graphql-unstable")
  -t, --token string      your token for using CircleCI
  -v, --verbose           Enable verbose logging.

orb validate

/tmp/tmp.7fGNduimQt => ./circleci help orb validate
validate an orb.yml

Usage:
  circleci orb validate [orb.yml] [flags]

Flags:
  -h, --help   help for validate

Global Flags:
  -e, --endpoint string   the endpoint of your CircleCI GraphQL API (default "https://circleci.com/graphql-unstable")
  -t, --token string      your token for using CircleCI
  -v, --verbose           Enable verbose logging.

See updated behavior here: #23 (comment)

marcomorain · 2018-07-24T09:01:10Z

cmd/cmd_suite_test.go

@@ -71,19 +71,25 @@ func (f tmpFile) write(fileContent string) error {
 	return err
 }

-func openTmpFile(path string) (tmpFile, error) {
+func openTmpDir(prefix string) (string, error) {


I don't think prefix is ever used here - copy + paste bug?

It's used here:
https://github.com/CircleCI-Public/circleci-cli/pull/23/files/1c600eaec59f3c63688668b38f7e6753e88de3bb#diff-bee1de2bffa142cede85d49cea1a90f7R76

So you can create a temp directory without specifying the prefix, just do openTmpDir("").

johnswanson force-pushed the read-from-stdin branch from 3c18bba to d2fff41 Compare July 16, 2018 23:07

zzak reviewed Jul 17, 2018

View reviewed changes

zzak suggested changes Jul 17, 2018

View reviewed changes

johnswanson force-pushed the read-from-stdin branch 2 times, most recently from 73df723 to 436d7bb Compare July 17, 2018 23:24

eric-hu self-requested a review July 18, 2018 17:19

eric-hu previously requested changes Jul 18, 2018

View reviewed changes

johnswanson and others added 11 commits July 24, 2018 17:52

allow reading config from stdin

affc4b5

make configPath and orbPath positional arguments

bfb14f1

- configPath has a default value of .circleci/config.yml - orbPath is required

add tests for reading orb from stdin

f7c85a4

Clean up expected json response data and match formatting from API

b0e3d2a

Remove irrelevant comment (makes linter happy)

7b0267e

💅 clean up unused type

8f46b58

Support default path for orb commands

cd59668

Also changes config commands to use a positional arg max of 1, and include it in usage documentation for `--help`. Ensure temp files and dirs are removed Also adds a test for testing orb validate with default path

Ensure temp orb is removed from default path test

1c600ea

zzak force-pushed the read-from-stdin branch from e265632 to 1c600ea Compare July 24, 2018 09:00

Allow default path for orb publish command as positional arg

f70fabd

zzak approved these changes Jul 24, 2018

View reviewed changes

zzak merged commit 7643b5e into master Jul 24, 2018

zzak deleted the read-from-stdin branch July 24, 2018 09:22

marcomorain reviewed Jul 24, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CIRCLE-12186] optionally read config file from stdin #23

[CIRCLE-12186] optionally read config file from stdin #23

johnswanson commented Jul 16, 2018

codecov-io commented Jul 16, 2018 •

edited

Loading

zzak Jul 17, 2018 •

edited

Loading

ndintenfass Jul 17, 2018

marcomorain Jul 17, 2018

zzak left a comment

johnswanson commented Jul 17, 2018

eric-hu left a comment

eric-hu Jul 18, 2018

eric-hu Jul 18, 2018

zzak commented Jul 23, 2018

zzak commented Jul 24, 2018

marcomorain Jul 24, 2018

zzak Jul 24, 2018

[CIRCLE-12186] optionally read config file from stdin #23

[CIRCLE-12186] optionally read config file from stdin #23

Conversation

johnswanson commented Jul 16, 2018

codecov-io commented Jul 16, 2018 • edited Loading

Codecov Report

zzak Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

ndintenfass Jul 17, 2018

Choose a reason for hiding this comment

marcomorain Jul 17, 2018

Choose a reason for hiding this comment

zzak left a comment

Choose a reason for hiding this comment

johnswanson commented Jul 17, 2018

eric-hu left a comment

Choose a reason for hiding this comment

eric-hu Jul 18, 2018

Choose a reason for hiding this comment

eric-hu Jul 18, 2018

Choose a reason for hiding this comment

zzak commented Jul 23, 2018

zzak commented Jul 24, 2018

config expand

config validate

orb expand

orb publish

orb validate

marcomorain Jul 24, 2018

Choose a reason for hiding this comment

zzak Jul 24, 2018

Choose a reason for hiding this comment

codecov-io commented Jul 16, 2018 •

edited

Loading

zzak Jul 17, 2018 •

edited

Loading