Create orb #32

zzak · 2018-07-25T08:09:22Z

I had accidentally merged #28 before #31 so going full circle here... :)

Co-authored-by: Hannah Henderson <[email protected]>

[CIRCLE-12217] Create orb from CLI

codecov-io · 2018-07-25T08:09:52Z

Codecov Report

❗ No coverage uploaded for pull request base (master@41b2030). Click here to learn what that means.
The diff coverage is 17.3%.

@@            Coverage Diff            @@
##             master      #32   +/-   ##
=========================================
  Coverage          ?   40.07%           
=========================================
  Files             ?       11           
  Lines             ?      776           
  Branches          ?        0           
=========================================
  Hits              ?      311           
  Misses            ?      442           
  Partials          ?       23

Impacted Files	Coverage Δ
cmd/diagnostic.go	`27.27% <ø> (ø)`
cmd/build.go	`34.32% <0%> (ø)`
cmd/orb.go	`26.95% <17.64%> (ø)`

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 41b2030...08bddb8. Read the comment docs.

marcomorain · 2018-07-25T10:20:11Z

api/api.go

@@ -63,7 +74,7 @@ func (response GQLResponseErrors) ToError() error {
 		messages = append(messages, response.Errors[i].Message)
 	}

-	return errors.New(strings.Join(messages, ": "))
+	return fmt.Errorf(strings.Join(messages, ": "))


Why the change from the errors package to string-formatting (fmt) package here? IMO using the errors package is more clear of the intent, and more clear still since there is no string-formatting going on.

This may be due to the nature of how these 2 PRs were merged, sorry about that.

I will fix it and address the other comments tomorrow unless @johnswanson or @hannahhenderson get to it.

The linter actually screamed at me for errors.New and demanded fmt.Errorf. I assumed that was because the error, in this case, is only a formatted string.

@hannahhenderson If linter warnings failing builds becomes a nuisance, I think we can check for them in bash and choose to not fail the build:

https://github.com/alecthomas/gometalinter#exit-status

marcomorain · 2018-07-25T10:23:36Z

api/api.go

@@ -243,7 +254,7 @@ func getOrganization(ctx context.Context, logger *logger.Logger, organizationNam
 	err := graphQLclient.Run(ctx, request, &response)

 	if err != nil || response.Organization.ID == "" {
-		err = errors.Wrap(err, fmt.Sprintf("Unable to find organization %s of vcs-type %s", organizationName, organizationVcs))
+		err = fmt.Errorf("Unable to find organization %s of vcs-type %s", organizationName, organizationVcs)


This change is destructive - the original error is lost. Let's suppose that error is a network failure, we want the error message to include this information, so that the user can take action.

Fair enough. This will need to become an if-else statement, then.

From what @johnswanson and I could tell, when the error is nil (like when the response is empty), if you wrap it with a string the end result is still nil (so the subsequent err != nil checks don't behave as-desired).

Do you know if there's a better way to handle this?

@marcomorain @hannahhenderson The change isn't destructive. These two lines have equivalent return values. fmt.Errorf returns an error object with string interpolation:

https://golang.org/pkg/fmt/#Errorf

See also: https://www.reddit.com/r/golang/comments/6ffrie/fmterrorf_or_errorsnew/

marcomorain · 2018-07-25T10:27:48Z

api/api.go

+// creating an orb
+type CreateOrbResponse struct {
+	Orb struct {
+		CreatedAt string


createdAt doesn't seem to be used anywhere. Let's remove from the GQL query and the response struct.

marcomorain · 2018-07-25T10:31:40Z

api/api.go

+
+	if err != nil || response.RegistryNamespace.ID == "" {
+		err = fmt.Errorf("Unable to find namespace %s", name)
+	}


A couple of comments here:

Let's split the case of when err is nil from the checking of response.RegistryNamespace.ID. We want to either return the error, or a wrapper around the error when it is not null.

When can response.RegistryNamespace.ID be empty/null? That seems like a fundamental failure in the API schema.

marcomorain · 2018-07-25T10:32:10Z

cmd/diagnostic.go

@@ -25,7 +25,7 @@ func diagnostic(cmd *cobra.Command, args []string) error {
 	Logger.Infof("GraphQL API endpoint: %s\n", endpoint)

 	if token == "token" || token == "" {
-		return errors.New("please set a token")
+		return fmt.Errorf("please set a token")


Why use a formatting function here for a static string?

marcomorain · 2018-07-25T10:32:46Z

api/api.go

+		return nil, err
+	}
+
+	return orb, err


If you return createOrbWithNsID(ctx, logger, name, namespaceID) then you can delete the next 5 lines of code.

marcomorain · 2018-07-25T10:35:55Z

api/api.go

+	err := graphQLclient.Run(ctx, request, &response)
+
+	if err != nil {
+		err = errors.Wrap(err, fmt.Sprintf("Unable to create orb %s for namespaceID %s", name, namespaceID))


In looking into the difference between fmt.Errorf and errors.New I learned that errors.Wrapf exists. That will neaten a bunch of code up.

hannahhenderson · 2018-07-25T15:44:32Z

@zzak #28 should have come before #31. This is the work for creating an orb, although it has a few string-updates for creating a namespace.

Update: Ohhhhhhhhhhh. I had #31 against #28 so you didn't have to look at as big a diff. Once #28 was merged, the base-branch for #31 could be changed to Master.

Please run `make dev` to get the latest version or `make lint` won't work for you.

We should come back to this and make sure addOrbElementsToBuffer error is handled properly.

WRT: error handling and unused createdAt in responses

Until this docker image is updated: https://github.com/supinf/dockerized-tools/tree/master/golang/gometalinter

All of Marc's comments were addressed and this PR should be ready to go after pairing with John.

hannahhenderson and others added 5 commits July 24, 2018 15:34

Add create orb

678be42

fix up create orb

388ec43

Co-authored-by: Hannah Henderson <[email protected]>

pass in namespace-qualified orb name

4f4813f

Co-authored-by: <[email protected]>

Lint file

2c5b820

Merge pull request #31 from CircleCI-Public/create-orb

4622b33

[CIRCLE-12217] Create orb from CLI

marcomorain previously requested changes Jul 25, 2018

View reviewed changes

hannahhenderson changed the title ~~Create namespace~~ Create orb Jul 25, 2018

Zachary Scott added 7 commits July 26, 2018 06:59

[CIRCLE-12673] Update gometalinter config to match renamed gosec

ea1e55f

Please run `make dev` to get the latest version or `make lint` won't work for you.

Make gosec linter happy

e22f63f

We should come back to this and make sure addOrbElementsToBuffer error is handled properly.

Apply feedback from Marc

b202629

WRT: error handling and unused createdAt in responses

Ensure latest gometalinter is installed for lint job

1f63f6f

Until this docker image is updated: https://github.com/supinf/dockerized-tools/tree/master/golang/gometalinter

supinf/gometalinter doesn't have git so use our golang image

232d2e0

Merge branch 'master' into create-namespace

c7a343f

Fix calling appendPostHandler after rebase

c89051d

Cleanup CreateOrb to remove unnecessary err check

08bddb8

johnswanson approved these changes Jul 25, 2018

View reviewed changes

zzak merged commit 0357eb2 into master Jul 25, 2018

zzak deleted the create-namespace branch July 25, 2018 23:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create orb #32

Create orb #32

zzak commented Jul 25, 2018 •

edited

Loading

codecov-io commented Jul 25, 2018 •

edited

Loading

marcomorain Jul 25, 2018

zzak Jul 25, 2018

hannahhenderson Jul 25, 2018

eric-hu Jul 26, 2018

marcomorain Jul 25, 2018

hannahhenderson Jul 25, 2018

eric-hu Jul 26, 2018

marcomorain Jul 25, 2018

marcomorain Jul 25, 2018

marcomorain Jul 25, 2018

marcomorain Jul 25, 2018

marcomorain Jul 25, 2018

hannahhenderson commented Jul 25, 2018 •

edited

Loading

Create orb #32

Create orb #32

Conversation

zzak commented Jul 25, 2018 • edited Loading

codecov-io commented Jul 25, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannahhenderson commented Jul 25, 2018 • edited Loading

zzak commented Jul 25, 2018 •

edited

Loading

codecov-io commented Jul 25, 2018 •

edited

Loading

hannahhenderson commented Jul 25, 2018 •

edited

Loading