Expose targets custom data in the NotaryRepository API #1146
Conversation
|
Please sign your commits following these rules: $ git clone -b "target-custom-data" [email protected]:ashfall/notary.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354207168
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -fAmending updates the existing PR. You DO NOT need to open a new one. |
ashfall
force-pushed
the
target-custom-data
branch
from
4fb7470 to
6e06a2b
Compare
ashfall
force-pushed
the
target-custom-data
branch
from
6e06a2b to
c59762d
Compare
ashfall
force-pushed
the
target-custom-data
branch
from
c59762d to
8be2b1d
Compare
client/client.go
Outdated
| var customData *canonicaljson.RawMessage | ||
| if target.Custom != nil { | ||
| customData = new(canonicaljson.RawMessage) | ||
| if err := customData.UnmarshalJSON(target.Custom); err != nil { |
There was a problem hiding this comment.
Is this just to validate that Target.Custom is actually valid json data and not a cast []byte?
There was a problem hiding this comment.
Awesome catch, thanks! In my first iteration, Target.Custom was a []byte, and this line was meant to do the correct type conversion. We shouldn't need this anymore. Updating!
There was a problem hiding this comment.
Pressed enter too soon. This was because I was getting "No error is expected but got json: error calling MarshalJSON for type *json.RawMessage: unexpected end of JSON input" in the cases where there was no custom data in the target, in the following line where we do metaJSON, err := json.Marshal(meta).
ashfall
force-pushed
the
target-custom-data
branch
from
821a643 to
1060e4f
Compare
client/client_test.go
Outdated
| require.True(t, reflect.DeepEqual(*currentTarget, newCurrentTarget.Target), "current target does not match") | ||
| } | ||
|
|
||
| func testListTargetWithCustom(t *testing.T, rootType string) { |
There was a problem hiding this comment.
Non-blocking: I wonder if it'd make sense to just fold this into testListTarget by adding custom metadata to one of the targets (either latest or custom)?
There was a problem hiding this comment.
Sure, that's a good idea. Updating, thanks!
cmd/notary/tuf.go
Outdated
| // Open and read a file containing the targetCustom data | ||
| func getTargetCustom(targetCustomFilename string) (json.RawMessage, error) { | ||
| var nilCustom json.RawMessage | ||
| targetCustomFile, err := os.Open(targetCustomFilename) |
There was a problem hiding this comment.
Non-blocking: ioutil.ReadFile might save some typing here, because it will open and close the file for you.
client/client.go
Outdated
| Name string // the name of the target | ||
| Hashes data.Hashes // the hash of the target | ||
| Length int64 // the size in bytes of the target | ||
| Custom canonicaljson.RawMessage // the custom data provided to describe the file at TARGETPATH |
There was a problem hiding this comment.
Non-blocking: would it be easier to make this a pointer, so that we can just assign to and from FileMeta without having check for nil?
cmd/notary/tuf.go
Outdated
| cmdTUFAdd := cmdTUFAddTemplate.ToCommand(t.tufAdd) | ||
| cmdTUFAdd.Flags().StringSliceVarP(&t.roles, "roles", "r", nil, "Delegation roles to add this target to") | ||
| cmdTUFAdd.Flags().BoolVarP(&t.autoPublish, "publish", "p", false, htAutoPublish) | ||
| cmdTUFAdd.Flags().StringVar(&t.custom, "custom", "", "Name of the file containing custom data for this target") |
There was a problem hiding this comment.
Non-blocking nit: Maybe "path to the file..." instead of "name of the file"?
cyli
left a comment
cyli
left a comment
There was a problem hiding this comment.
LGTM! Thanks for adding this feature!
cmd/notary/tuf.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| err = json.Unmarshal(rawTargetCustom, &targetCustom) |
There was a problem hiding this comment.
We should be doing targetCustom.Unmarshal(rawTargetCustom). That will also require targetCustom to have been initialized, so line 258 needs to be targetCustom := new(json.RawMessage) then you'll also be able to remove the & when you reference it because new will already return a reference.
cmd/notary/tuf.go
Outdated
| "github.com/docker/distribution/registry/client/auth/challenge" | ||
| "github.com/docker/distribution/registry/client/transport" | ||
| "github.com/docker/go-connections/tlsconfig" | ||
| "github.com/docker/go/canonical/json" |
There was a problem hiding this comment.
let's import this aliased to canonicaljson for clarity. It reduces confusion when/if somebody is having type errors related to the encoding/json vs .../canonical/json libraries
ashfall
force-pushed
the
target-custom-data
branch
from
10990ff to
1f4822b
Compare
- Update Target definition and NewTarget to store custom data - tufAdd and tufAddByHash now take a filename as a flag, open and read the file, and pass the custom data bytes to NewTarget. - Client APIs now understand Target.Custom - Test that client lists and gets targets with custom data - getTargetCustom should return a nil RawMessage, not a []byte - Test tufAdd with target custom data - Trigger a usage error for the tufLookup command in a test Signed-off-by: Ashwini Oruganti <[email protected]>
Signed-off-by: Ashwini Oruganti <[email protected]>
Signed-off-by: Ashwini Oruganti <[email protected]>
Signed-off-by: Ashwini Oruganti <[email protected]>
Signed-off-by: Ashwini Oruganti <[email protected]>
Signed-off-by: Ashwini Oruganti <[email protected]>
Signed-off-by: Ashwini Oruganti <[email protected]>
Signed-off-by: Ashwini Oruganti <[email protected]>
Signed-off-by: Ashwini Oruganti <[email protected]>
To avoid confusing it with encoding/json. Signed-off-by: Ashwini Oruganti <[email protected]>
ashfall
force-pushed
the
target-custom-data
branch
from
1f4822b to
a6089e0
Compare
4 participants
Closes #1143