Rewrite Claims to support updated spec #212

carolynvs · 2020-05-29T19:47:17Z

This is basically a rewrite, sorry. It updates our claims implementation to support the changes to the claims spec per cnab-claim-1.0.0-DRAFT+b5ed2f3

I apologize that this is a monolith PR. I found it impossible to break up due to the nature of the changes to the spec and how it impacted the codebase.

This is not an exhaustive list of changes, see the spec for that but here are the major changes:

Split Result from Claim
Do not persist outputs on Claim. Now they are persisted independently, and not all outputs have to be persisted, it's up to the tooling.
Store output's content digest on the Result (see How to determine which outputs to retrieve? #210 for an open question about if we need to store every output's digest or just the ones we are persisting).
Claim represents all the inputs to an operation, it doesn't change after an operation is performed. Result contains the status of the operation, and there can now be more than one so that we can keep track of its progress, e.g. Pending -> Running -> Succeeded.
We don't reuse the same claim document for persistence anymore, preserving an audit trail / history. crud.Store and its implementations had to change it's API to support this, and querying by a "tag" or "foreign key" so that we could look for records by its key (e.g. claim id) or find all records by the "foreign key" such as installation name. While also supporting data types like credentials that do not need this.
The solution caters to the lowest common denominator, filesystem, and in the future we could introduce interfaces to help declare that an implementation supports querying (basically any other storage type like mongo or a real database).
Expanded the claim store to support Result and Outputs and a new type and isn't persisted but is useful for everyone: installation. Installation represents an installation of a bundle (a set of claims).
Claim store has built-in support for encrypting claim data and sensitive outputs. Claim should have always been encrypted or securely stored per the spec because they store sensitive data, such as database passwords. Now it's easier/encouraged for tools to implement that.
Collapsed the implementation for actions into a single class Action without an interface since it wasn't being used. Action now has support for working with claims to help coordinate properly persisting them, especially when errors occur.
Added example godoc demonstrating how to use Action and persist claim data properly in various scenarios.
The logic around managing and defaulting outputs was simplified. Drivers don't have to deal with defaulting missing outputs or checking or missing required outputs, the runtime handles that now. Outputs are managed by name on OperationResult.

🚨 Based on PR #211, will need to rebase when that merges.

vdice

PR still in draft, but noted a few items, all minor. This is looking really good!

action/action.go

claim/claim.go

claim/claim_test.go

claim/result.go

driver/command/command_nix_test.go

driver/debug/debug_test.go

driver/driver.go

vdice

All of my initial review comments have been addressed. Thank you! However, I remembered we are still skipping the claim schema test. Should we unskip and update in this PR? Or relegate to a follow-up?

carolynvs · 2020-06-08T14:55:23Z

Whooooooo! I unskipped the test AND IT PASSED! The ultimate validation. 😀 Thanks for remembering that it was skipped.

Split result into separate struct where claim contains an action's input and the result contains the results (status, outputs, etc) of the operation. Since each action has all the same logic, and they were also copying each other, including tests, I consolidate that into a single function that they all share. * encryption in claim store * run doesn't modify claim * switch cred.bundle to value * Operation.Outputs map[PATH]NAME so that the operation knows which outputs to grab, but from then on in the operation result we can just deal with the output name * Don't make each driver deal with output defaults, have that be something that the runtime implementation (action) handles after the driver is executed. Signed-off-by: Carolyn Van Slyck <[email protected]>

* Make fields that aren't always set unexported, e.g. Result.claim, so that people don't accidently use them inappropriately. * Remove todos that were turned into issues * Move retrieval of most recent output values from installation to the claim store. * Add more claim store unit tests * Fix docker integration test Signed-off-by: Carolyn Van Slyck <[email protected]>

Signed-off-by: Carolyn Van Slyck <[email protected]>

Check errors using contains not direct comparisons so that it can deal with wrapped errors. Signed-off-by: Carolyn Van Slyck <[email protected]>

Signed-off-by: Carolyn Van Slyck <[email protected]>

carolynvs · 2020-06-08T14:56:30Z

Rebased with changes from #211 that just merged.

Use pointers to manage optionality of some fields on Result and Claim to indicate that they may not always be set and must be nil checked before assuming that they are safe to use, or assume that the length is meaningful. Signed-off-by: Carolyn Van Slyck <[email protected]>

radu-matei · 2020-06-15T16:31:03Z

action/example_install_test.go

+)
+
+// Install the bundle and only record the success/failure of the operation.
+func Example_install() {


nit: I think we've used Example_Install as convention for tests before (looking at, for example, TestClaim_Validate).

Oh yeah so I really wanted to name it that too, but then it doesn't show up in the godoc! 😅 It only shows up when lower-cased. I'm not sure why but that's why I named the whole file examples that way.

In the "doc" for godoc, they tell you to look at the sort package and that's what they do there too

https://blog.golang.org/examples
https://golang.org/src/sort/example_keys_test.go

If you know how to get it working with upper-case, let me know!

Have we tried ExampleInstall?

Yup! godoc doesn't like that either 🤷‍♀️

action/wellknown.go

claim/provider.go

radu-matei

This has so many great changes, thanks a LOT for the work you put into this!

I have a few very small comments, but otherwise LGTM.

Signed-off-by: Carolyn Van Slyck <[email protected]>

jlegrone · 2020-06-16T20:47:48Z

driver/debug/debug.go

+}
+
+// Run executes the operation on the Debug driver
+func (d *Driver) Run(op *driver.Operation) (driver.OperationResult, error) {


Can we pass a context object as well to handle things like deadlines/cancellation/tracing?

Suggested change

func (d *Driver) Run(op *driver.Operation) (driver.OperationResult, error) {

func (d *Driver) Run(ctx context.Context, op *driver.Operation) (driver.OperationResult, error) {

I started making this change here, but I think it would be better in a second PR so we can really look over the changes to implementing the context handling in the drivers in isolation. I'll merge this then immediately open up the PR with my changes and tests so far and we can see what else needs to be done.

Signed-off-by: Carolyn Van Slyck <[email protected]>

vdice reviewed Jun 1, 2020

View reviewed changes

This was referenced Jun 2, 2020

Use concurrency in the data persistence layer? #214

Closed

Return partial data on error? #215

Open

Consolidate regex for Claim Name Validation Regex #217

Open

carolynvs force-pushed the claims-claims-the-musical-fruit branch 5 times, most recently from df6fff0 to 4d7c9e0 Compare June 4, 2020 00:21

carolynvs marked this pull request as ready for review June 4, 2020 13:12

carolynvs mentioned this pull request Jun 4, 2020

Use latest claims code in cnab-go getporter/porter#1065

Merged

3 tasks

vdice reviewed Jun 6, 2020

View reviewed changes

carolynvs-msft added 5 commits June 8, 2020 09:55

Add tests for output defaulting

5da84ec

Signed-off-by: Carolyn Van Slyck <[email protected]>

Harden not found checks

36ff203

Check errors using contains not direct comparisons so that it can deal with wrapped errors. Signed-off-by: Carolyn Van Slyck <[email protected]>

Unskip the claim schema test

1f6d106

Signed-off-by: Carolyn Van Slyck <[email protected]>

carolynvs force-pushed the claims-claims-the-musical-fruit branch from 676b539 to 1f6d106 Compare June 8, 2020 14:56

vdice approved these changes Jun 8, 2020

View reviewed changes

vdice requested a review from radu-matei June 10, 2020 19:22

carolynvs requested a review from jlegrone June 11, 2020 18:02

radu-matei reviewed Jun 15, 2020

View reviewed changes

action/wellknown.go Outdated Show resolved Hide resolved

radu-matei reviewed Jun 15, 2020

View reviewed changes

claim/provider.go Outdated Show resolved Hide resolved

radu-matei approved these changes Jun 15, 2020

View reviewed changes

Incorporate feedback

7388019

Signed-off-by: Carolyn Van Slyck <[email protected]>

jlegrone reviewed Jun 16, 2020

View reviewed changes

carolynvs force-pushed the claims-claims-the-musical-fruit branch from f68a445 to 5624fb7 Compare June 16, 2020 20:48

Add godoc for the claims package

f33d0f8

Signed-off-by: Carolyn Van Slyck <[email protected]>

carolynvs force-pushed the claims-claims-the-musical-fruit branch from 5624fb7 to f33d0f8 Compare June 16, 2020 21:33

carolynvs merged commit fedd74f into cnabio:master Jun 16, 2020

carolynvs deleted the claims-claims-the-musical-fruit branch May 8, 2021 12:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite Claims to support updated spec #212

Rewrite Claims to support updated spec #212

carolynvs commented May 29, 2020 •

edited

Loading

vdice left a comment

vdice left a comment •

edited

Loading

carolynvs commented Jun 8, 2020 •

edited

Loading

carolynvs commented Jun 8, 2020

radu-matei Jun 15, 2020

carolynvs Jun 15, 2020

jlegrone Jun 16, 2020

carolynvs Jun 16, 2020

radu-matei left a comment

jlegrone Jun 16, 2020

carolynvs Jun 16, 2020

	func (d Driver) Run(op driver.Operation) (driver.OperationResult, error) {
	func (d Driver) Run(ctx context.Context, op driver.Operation) (driver.OperationResult, error) {

Rewrite Claims to support updated spec #212

Rewrite Claims to support updated spec #212

Conversation

carolynvs commented May 29, 2020 • edited Loading

vdice left a comment

Choose a reason for hiding this comment

vdice left a comment • edited Loading

Choose a reason for hiding this comment

carolynvs commented Jun 8, 2020 • edited Loading

carolynvs commented Jun 8, 2020

radu-matei Jun 15, 2020

Choose a reason for hiding this comment

carolynvs Jun 15, 2020

Choose a reason for hiding this comment

jlegrone Jun 16, 2020

Choose a reason for hiding this comment

carolynvs Jun 16, 2020

Choose a reason for hiding this comment

radu-matei left a comment

Choose a reason for hiding this comment

jlegrone Jun 16, 2020

Choose a reason for hiding this comment

carolynvs Jun 16, 2020

Choose a reason for hiding this comment

carolynvs commented May 29, 2020 •

edited

Loading

vdice left a comment •

edited

Loading

carolynvs commented Jun 8, 2020 •

edited

Loading