Skip to content
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

Circle-11123 publish orb #27

Merged
merged 10 commits into from
Jul 20, 2018
Merged

Circle-11123 publish orb #27

merged 10 commits into from
Jul 20, 2018

Conversation

eric-hu
Copy link
Contributor

@eric-hu eric-hu commented Jul 18, 2018

User can publish an orb from the CLI

$ go run main.go orb publish --path ./tmp-orb.yml -i bb604b45-b6b0-4b81-ad80-796f15eddf87 -o 0.0.2
Orb published

$ go run main.go orb publish --path ./tmp-orb.yml -i bb604b45-b6b0-4b81-ad80-796f15eddf87 -o 0.0.2
Error: orb revision already exists exit status 255

@eric-hu
Copy link
Contributor Author

eric-hu commented Jul 18, 2018

Feature-complete, but currently refactoring a lot of copy-pasted code.

Ideally 3 things I'd like to DRY up:

  • error handling
  • test suite mock server setup
  • make graphql response mocks human-readable (eliminate the need for \n and \t)

Two feature behaviors to change: --version and --orb-name should be a positional arguments

api/api.go Outdated
@@ -35,6 +48,18 @@ func (response ConfigResponse) ToError() error {
return errors.New(strings.Join(messages, ": "))
}

// ToError (PublishOrbResponse) returns all errors that the GraphQL API returns
// as part of a publish orb request.
func (response PublishOrbResponse) ToError() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have 2 copies of this function - we can extract out a struct that has:

	Errors []struct {
		Message string
	}

and use this a mix-in for any responses that have this shape of data. Then we can have a single ToError function.

api/api.go Outdated
err = graphQLclient.Run(ctx, request, response)

if err != nil {
return errors.Wrap(err, "Unable to validate config")
Copy link
Contributor

Choose a reason for hiding this comment

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

"Unable to validate config" - copy paste error?

api/api.go Outdated
}
}

return &response.PublishOrb.PublishOrbResponse, publishOrbQuery(ctx, logger,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would delete the publishOrbQuery function and just put the body of it here in OrbPublish. The reason for the split in the config validation code-path is because I wanted to re-use a large chunk of code that was copy+pasted between orb validate and config validate.

@codecov-io
Copy link

Codecov Report

Merging #27 into master will increase coverage by 1.89%.
The diff coverage is 45.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #27      +/-   ##
==========================================
+ Coverage    41.6%   43.49%   +1.89%     
==========================================
  Files          11       10       -1     
  Lines         661      646      -15     
==========================================
+ Hits          275      281       +6     
+ Misses        363      343      -20     
+ Partials       23       22       -1
Impacted Files Coverage Δ
cmd/orb.go 31.69% <45.83%> (+8.87%) ⬆️
cmd/root.go 74.5% <0%> (-0.5%) ⬇️
cmd/version.go

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 143611a...642314a. Read the comment docs.

@eric-hu
Copy link
Contributor Author

eric-hu commented Jul 19, 2018

Pushed changes Marc suggested.

Didn't have time to get around to these two tasks:

  • test suite mock server setup
  • make graphql response mocks human-readable (eliminate the need for \n and \t)

Created a card to capture this task: https://circleci.atlassian.net/browse/CIRCLE-12582

@marcomorain marcomorain merged commit 6d28439 into master Jul 20, 2018
@marcomorain
Copy link
Contributor

nice one

@marcomorain marcomorain deleted the 11123-publish-orb branch July 20, 2018 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants