Minor refactoring (nicer lib usage) #1081

ecordell · 2017-01-10T23:48:07Z

When using Notary as a library, I ran into some visibility issues that I've addressed here.

I'm a little uneasy about this PR because I can only justify it from my own experience, but I don't think there's anything egregious here. Personally, I think this changeset a slight readability improvement, regardless.

docker-jenkins · 2017-01-10T23:48:09Z

Can one of the admins verify this patch?

cyli · 2017-01-10T23:52:32Z

jenkins, test this please

HuKeping · 2017-01-11T14:18:43Z

server/server.go

 	invalidGUNErr := errors.ErrInvalidGUN.WithDetail(fmt.Sprintf("Require GUNs with prefix: %v", repoPrefixes))
 	notFoundError := errors.ErrMetadataNotFound.WithDetail(nil)

 	r := mux.NewRouter()
 	r.Methods("GET").Path("/v2/").Handler(authWrapper(handlers.MainHandler))
-
-	r.Methods("POST").Path("/v2/{imageName:[^*]+}/_trust/tuf/").Handler(createHandler(_serverEndpoint{
+	r.Methods("POST").Path("/v2/{imageName:[^*]+}/_trust/tuf/").Handler(CreateHandler(EndpointConfig{


How about changing the imageName to gun in this PR as well, see #1069

cyli · 2017-01-12T00:47:53Z

jenkins, test this please

cyli · 2017-01-12T00:49:18Z

server/storage/sql_models.go

@@ -11,6 +11,16 @@ const (
 	changeCategoryDeletion = "deletion"
 )

+// TUFFileTableName returns the name used for the tuf file table
+var TUFFileTableName = func() string {


Non-blocking: perhaps these should just be string constants?

As they're not modifiable, yeah, they should be string consts.

Then again, they should also NEVER be used in more than one context (i.e. through the appropriate struct) so I actually prefer them being defined directly in the struct methods. It tightly binds them to the correct context.

@ecordell I think these functions are the only thing that I really don't like in this PR. the rest of it I'm fine with (I've been waiting to see if you wanted to make the EndpointConfig updates here).

I really preferred them attached to the struct, it keeps it very clear what it's appropriate to use them for. However, if you want to break them out, it makes a lot more sense to just make them const's.

Agreed with @endophage , I don't see the explicit benefit we can from these functions, it's not that directly and clear.

Didn't mean to let this stagnate, I've been trying to evaluate which of these changes are really needed.

I was thinking that the functions might help for threshold "witnessing" (or whatever the term is now) by creating a different table for "staged" metadata. Constants would also work for that, though. Since that design is still very up in the air I will change this and update the PR.

cyli · 2017-01-12T00:49:53Z

utils/http.go

 		ctxu.GetResponseLogger(ctx).Info("response completed")
-	}()
+	}(ctx)


👍 Thank you for this fix!

Is this correct? Don't we want to use the possibly modified context (which may contain additional info) when the function exits?

doAuth does not currently updates the logger, but I suppose it's possible in the future that it may. I'm not sure if we necessarily care though about all the other fields in a "response completed" log.

You may be right and we may want the modified context if the logger gets updated in the future. I have no strong opinions either way. I had originally assumed we just wanted the base logger. :)

endophage · 2017-01-12T01:09:35Z

I'm curious what the use case is here. My assumption is that you're adding additional http routes to the server (on the object returned by RootHandler), and want to apply the same EntrypointConfig over those additional routes?

riyazdf · 2017-01-12T03:58:11Z

Piggy-backing off of @endophage's comment - I'm cool with making createHandler exported, but I want to make sure that if it's because Notary could benefit from a particular endpoint that we can add that functionality to the released server?

endophage · 2017-01-12T19:56:03Z

On a related note, I'm wondering if EntrypointConfig has outlived its usefulness??? We construct a config inline to the CreateHandler function call only to immediately deconstruct it within the CreateHandler function. Should we just be passing the fields as arguments directly? Admittedly the argument list would be long-ish but it would be less obscure.

I think at some point (scraping my memory) we did it so you didn't end up with CreateHandler(nil, nil, nil, thingICareAbout, nil, nil, nil, nil), but looking at the handlers, only one directly sets 5 fields, and the rest all appear to set 6+ out of a total 8 fields.

If we think it's still useful, maybe it makes sense to pull out the 3 fields every entrypoint is using: OperationName, ServerHandler, and PermissionsRequired and make those arguments to CreateHandler

ecordell · 2017-02-18T00:01:18Z

Took a while to get back around to this, but I believe I've addressed everything now!

endophage · 2017-02-22T17:50:19Z

server/server.go

@@ -116,13 +116,27 @@ func filterImagePrefixes(requiredPrefixes []string, err error, handler http.Hand
 	})
 }

-type _serverEndpoint struct {
+// EndpointConfig stores settings used to create a ServerHandler
+type EndpointConfig struct {


We can delete this now right?

Yup, I think the rebase added it back...

endophage · 2017-02-22T17:50:34Z

server/server.go

+	RepoPrefixes        []string
+}
+
+// CreateHandler creates a server handler from an EndpointConfig


Comment needs update now that we don't take an EndpointConfig any more

Signed-off-by: Evan Cordell <[email protected]>

endophage · 2017-02-23T16:32:08Z

server/storage/sql_models.go

@@ -11,6 +11,12 @@ const (
 	changeCategoryDeletion = "deletion"
 )

+// TUFFileTableName returns the name used for the tuf file table
+var TUFFileTableName = "tuf_files"


Signed-off-by: Evan Cordell <[email protected]>

endophage · 2017-02-23T20:16:56Z

Jenkins had a random Java failure. I've given it a kick. I'm good to go on this once Jenkins goes green.

endophage · 2017-02-23T20:21:45Z

@cyli @riyazdf @HuKeping can we get another LGTM?

cyli · 2017-02-23T21:49:41Z

LGTM!

GordonTheTurtle added the dco/no label Jan 10, 2017

ecordell force-pushed the lib-fixes branch from ca083ed to 42d8809 Compare January 10, 2017 23:48

GordonTheTurtle removed the dco/no label Jan 10, 2017

ecordell force-pushed the lib-fixes branch from 42d8809 to e45cbff Compare January 10, 2017 23:50

ecordell force-pushed the lib-fixes branch 3 times, most recently from d5fa7ff to c78d906 Compare January 11, 2017 14:17

HuKeping reviewed Jan 11, 2017

View reviewed changes

cyli reviewed Jan 12, 2017

View reviewed changes

ecordell force-pushed the lib-fixes branch from 3816240 to 814b67d Compare February 17, 2017 23:59

GordonTheTurtle added the dco/no label Feb 17, 2017

ecordell force-pushed the lib-fixes branch from 814b67d to bfe54aa Compare February 18, 2017 00:00

GordonTheTurtle removed the dco/no label Feb 18, 2017

GordonTheTurtle added the dco/no label Feb 22, 2017

ecordell force-pushed the lib-fixes branch from 6cc6630 to 200eb3d Compare February 22, 2017 16:14

GordonTheTurtle removed the dco/no label Feb 22, 2017

endophage suggested changes Feb 22, 2017

View reviewed changes

ecordell added 5 commits February 23, 2017 07:38

small refactoring to make it easier to use notary as a library

f7de237

Signed-off-by: Evan Cordell <[email protected]>

Rename imageName -> gun

df0a50d

Signed-off-by: Evan Cordell <[email protected]>

revert table names to string constants

fe6bcf2

Signed-off-by: Evan Cordell <[email protected]>

get rid of EndpointConfig

f86baa5

Signed-off-by: Evan Cordell <[email protected]>

remove unused EndpointConfig

0b042e9

Signed-off-by: Evan Cordell <[email protected]>

ecordell force-pushed the lib-fixes branch from 109fd2f to 0b042e9 Compare February 23, 2017 12:38

endophage suggested changes Feb 23, 2017

View reviewed changes

const table names

e7749ff

Signed-off-by: Evan Cordell <[email protected]>

endophage approved these changes Feb 23, 2017

View reviewed changes

endophage merged commit b5a20a3 into notaryproject:master Feb 23, 2017

ecordell deleted the lib-fixes branch March 2, 2017 13:12

ecordell mentioned this pull request Apr 21, 2017

Use GUN instead of ImageName to show that notary is not only for docker image #1069

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor refactoring (nicer lib usage) #1081

Minor refactoring (nicer lib usage) #1081

ecordell commented Jan 10, 2017

docker-jenkins commented Jan 10, 2017

cyli commented Jan 10, 2017

HuKeping Jan 11, 2017

cyli commented Jan 12, 2017

cyli Jan 12, 2017

endophage Jan 12, 2017

endophage Jan 12, 2017

endophage Feb 16, 2017

HuKeping Feb 16, 2017

ecordell Feb 16, 2017 •

edited

Loading

cyli Jan 12, 2017

endophage Jan 12, 2017

cyli Jan 12, 2017

cyli Jan 12, 2017

endophage commented Jan 12, 2017

riyazdf commented Jan 12, 2017 •

edited

Loading

endophage commented Jan 12, 2017 •

edited

Loading

ecordell commented Feb 18, 2017

endophage Feb 22, 2017

ecordell Feb 22, 2017

ecordell Feb 22, 2017

endophage Feb 22, 2017

ecordell Feb 22, 2017

endophage Feb 23, 2017

ecordell Feb 23, 2017

endophage commented Feb 23, 2017

endophage commented Feb 23, 2017

cyli commented Feb 23, 2017

Minor refactoring (nicer lib usage) #1081

Minor refactoring (nicer lib usage) #1081

Conversation

ecordell commented Jan 10, 2017

docker-jenkins commented Jan 10, 2017

cyli commented Jan 10, 2017

Choose a reason for hiding this comment

cyli commented Jan 12, 2017

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

ecordell Feb 16, 2017 • edited Loading

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

endophage commented Jan 12, 2017

riyazdf commented Jan 12, 2017 • edited Loading

endophage commented Jan 12, 2017 • edited Loading

ecordell commented Feb 18, 2017

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

endophage commented Feb 23, 2017

endophage commented Feb 23, 2017

cyli commented Feb 23, 2017

ecordell Feb 16, 2017 •

edited

Loading

riyazdf commented Jan 12, 2017 •

edited

Loading

endophage commented Jan 12, 2017 •

edited

Loading