Enable custom actions on existing claims or bundles #870

youreddy · 2019-09-27T00:38:46Z

After reading this issue comment I took a look at how we could make it possible to run custom actions on bundles that duffle has built but not installed or bundles it does not know about. Since running a custom action currently requires a claim, the easiest thing to do was create a claim if one did not exist.

This PR allows duffle run to perform custom actions in the following scenarios

When the claim exists:
users can run duffle run someCustomAction--claim existingClaim and duffle will reference the claim store in order to perform the custom action.
When the claim does not exist but duffle knows about the bundle:
users can run duffle run someCustomAction --bundle someBundle --claim myNewClaim and duffle will look in the store to perform the custom action. It will also create a claim if the action is a modifying one.
When the claim doe not exist and duffle does not know about the bundle:
users can run duffle run someCustomAction --bundle-is-file some-bundle.json --claim myNewClaim and duffle will use the bundle definition to run the custom action. It will also create a claim if the action is a modifying one.

Let me know what you all think of this functionality. I'm open to feedback and restructuring the design of this cli command.

Related Issues:

- "duffle run" requires an action and claim name - if a claim does not exist, the user can specify a new claim to create and the bundle. - the bundle can either be the path to a bundle.json file or the name of a bundle in duffle's store - add tests for run command and error cases

youreddy · 2019-09-30T17:48:28Z

Hi @radu-matei @glyn @jeremyrickard @carolynvs! Would be great to get some 👀 on this one when you get a chance.

glyn

I'm a bit fuzzy about the design, but I've never really had a clear understanding of how claims work, so please bear with me.

I thought that the existence of a claim implies the bundle had been installed and that the claim would be deleted at uninstall. The approach of creating new claims just to run custom actions seems to change that and might cause issues. For instance, if I run a custom claim after uninstalling a bundle and a claim is created, how can I clean up the claim? Is it possible to run uninstall successfully again? Also, what happens when install is run and the claim already exists - shouldn't the install fail because the bundle is already installed?

Also, it seems slightly odd that modifying custom actions should save their claims, but non-modifying actions shouldn't as that makes the lifecycle of claims even more complicated to reason about.

glyn · 2019-10-01T06:02:15Z

cmd/duffle/run.go

-	flags.StringArrayVarP(&credentialsFiles, "credentials", "c", []string{}, "Specify a set of credentials to use inside the CNAB bundle")
-	flags.StringVarP(&valuesFile, "parameters", "p", "", "Specify file containing parameters. Formats: toml, MORE SOON")
-	flags.StringArrayVarP(&setParams, "set", "s", []string{}, "Set individual parameters as NAME=VALUE pairs")
+	flags.StringVarP(&run.claimName, "claim", "i", "", "Specify the name of an existing claim (required)")


Is this always an existing claim?

glyn · 2019-10-01T06:03:36Z

cmd/duffle/run.go

+	flags.StringArrayVarP(&run.setParams, "set", "s", []string{}, "Set individual parameters as NAME=VALUE pairs")
+
+	err := cmd.MarkFlagRequired("claim")
+	if err != nil {


nit: in duffle, it's more usual to fold assignments and if statements together when no new variables need to last beyond the scope of the if.

glyn · 2019-10-01T06:08:22Z

cmd/duffle/run.go

+	}
+
+	if r.bundlePath != "" {
+		return r.createClaimFromBundlePath()


What happens here if a claim already exists?

youreddy · 2019-10-01T21:00:47Z

Thanks for taking a look @glyn!

The approach of creating new claims just to run custom actions seems to change that and might cause issues.

I was going back and forth on this. Attaching a bundle definition to a new claim was the easiest way to maintain the current interface for creating a driver operation. But I could have also implemented this with a temporary claim that served the purpose of creating the operation and then not persist the claim in duffle's store. This would maintain the constraint that claims should only be created on install. I ended up keeping the claim around because it was the only way for a user to keep track of which action had been run and its status.

The delete implications are valid concerns and I would be happy to make changes that didn't persist the claim in the duffle store. I think that's the right thing to do since the spec specifically defines claims as installed bundles. How would you feel about the PR if I made that change?

Also, it seems slightly odd that modifying custom actions should save their claims, but non-modifying actions shouldn't as that makes the lifecycle of claims even more complicated to reason about.

This was just to maintain the current logic here.

glyn · 2019-10-02T14:44:12Z

Thanks for taking a look @glyn!

The approach of creating new claims just to run custom actions seems to change that and might cause issues.

I was going back and forth on this. Attaching a bundle definition to a new claim was the easiest way to maintain the current interface for creating a driver operation.

I've been looking at this and it appears that runCmd.claimName will be the empty string in cases where you have to "new up" a claim from the bundle. I think the namespace for claim names is global, so such claims, if persisted, would tend to clobber each other. This doesn't feel good.

But I could have also implemented this with a temporary claim that served the purpose of creating the operation and then not persist the claim in duffle's store. This would maintain the constraint that claims should only be created on install. I ended up keeping the claim around because it was the only way for a user to keep track of which action had been run and its status.

Wouldn't it be better to change the contract and admit that there may not be a claim when a custom action is run against a bundle which is not currently installed?

The delete implications are valid concerns and I would be happy to make changes that didn't persist the claim in the duffle store. I think that's the right thing to do since the spec specifically defines claims as installed bundles. How would you feel about the PR if I made that change?

I think that would be an improvement, but it may not go far enough (see above).

Also, it seems slightly odd that modifying custom actions should save their claims, but non-modifying actions shouldn't as that makes the lifecycle of claims even more complicated to reason about.

This was just to maintain the current logic here.

Ok. It's not obvious to me that this logic needs to be preserved in this case.

youreddy · 2019-10-02T19:24:22Z

I've been looking at this and it appears that runCmd.claimName will be the empty string in cases where you have to "new up" a claim from the bundle. I think the namespace for claim names is global, so such claims, if persisted, would tend to clobber each other. This doesn't feel good.

Right, so I think we're moving away from the idea of persisting it and we can use a randomly generated string for the temporary claim name. This would be sufficient for creating the driver operation and would it make it so that the claim name is only required if it already exists.

Wouldn't it be better to change the contract and admit that there may not be a claim when a custom action is run against a bundle which is not currently installed?

Can you expand on this? By admit, do you mean providing some log output that the claim does not exist and ask the user to retry with a bundle reference?

glyn · 2019-10-03T09:09:41Z

I've been looking at this and it appears that runCmd.claimName will be the empty string in cases where you have to "new up" a claim from the bundle. I think the namespace for claim names is global, so such claims, if persisted, would tend to clobber each other. This doesn't feel good.

Right, so I think we're moving away from the idea of persisting it and we can use a randomly generated string for the temporary claim name. This would be sufficient for creating the driver operation and would it make it so that the claim name is only required if it already exists.

Wouldn't it be better to change the contract and admit that there may not be a claim when a custom action is run against a bundle which is not currently installed?

Can you expand on this? By admit, do you mean providing some log output that the claim does not exist and ask the user to retry with a bundle reference?

No, I meant that we should admit the facts of the situation to the custom action which will then see an empty string as a claim name and understand that the bundle isn't installed and that there isn't a claim.

Specifically, I think we need to provide a way to create a driver operation when there isn't a claim.

youreddy · 2019-10-08T15:28:33Z

Specifically, I think we need to provide a way to create a driver operation when there isn't a claim.

Yeah, that was the larger change I was trying to avoid. Especially since test coverage throughout the code base is inconsistent. I'll revisit this PR and see what it would take to make that change.

glyn reviewed Oct 1, 2019

View reviewed changes

carolynvs mentioned this pull request Dec 4, 2019

Support running custom actions without installing the bundle getporter/porter#821

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable custom actions on existing claims or bundles #870

Enable custom actions on existing claims or bundles #870

youreddy commented Sep 27, 2019

youreddy commented Sep 30, 2019

glyn left a comment

glyn Oct 1, 2019

glyn Oct 1, 2019

glyn Oct 1, 2019

youreddy commented Oct 1, 2019

glyn commented Oct 2, 2019

youreddy commented Oct 2, 2019

glyn commented Oct 3, 2019

youreddy commented Oct 8, 2019

Enable custom actions on existing claims or bundles #870

Are you sure you want to change the base?

Enable custom actions on existing claims or bundles #870

Conversation

youreddy commented Sep 27, 2019

youreddy commented Sep 30, 2019

glyn left a comment

Choose a reason for hiding this comment

glyn Oct 1, 2019

Choose a reason for hiding this comment

glyn Oct 1, 2019

Choose a reason for hiding this comment

glyn Oct 1, 2019

Choose a reason for hiding this comment

youreddy commented Oct 1, 2019

glyn commented Oct 2, 2019

youreddy commented Oct 2, 2019

glyn commented Oct 3, 2019

youreddy commented Oct 8, 2019