search

cnabio / duffle

CNAB installer
https://duffle.sh
MIT License
375 stars 54 forks source link

Enable custom actions on existing claims or bundles #870

Open youreddy opened 5 years ago

youreddy commented 5 years ago

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

  1. 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.

  2. 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.

  3. 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:

youreddy commented 5 years ago

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

youreddy commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.