Skip to content
This repository has been archived by the owner on May 3, 2022. It is now read-only.

Add support for stateless actions #630

Merged
merged 2 commits into from
Mar 14, 2019

Conversation

simonferquel
Copy link
Contributor

This implements stateless actions as defined in cnabio/cnab-spec#85.

@radu-matei
Copy link
Member

Ok, this is in more advanced state than https://github.com/radu-matei/duffle/tree/feat-629-sync-action-spec, so I'll close that one.

Just for future reference, let's make sure we assign an issue before starting the work on it, so we don't duplicate work 😃

pkg/bundle/bundle.go Outdated Show resolved Hide resolved
pkg/action/install.go Outdated Show resolved Hide resolved
@simonferquel
Copy link
Contributor Author

Sorry about the duplicated work. Fixed the annotations and added a notStateless const for more clarity

@radu-matei
Copy link
Member

This looks good, although I have a question (that I think will result in an issue for Duffle) - according to the spec for stateless actions: The action does not act on a claim - however, duffle run requires an installation name, and does not accept a bundle.

If I understand the usage of stateless actions correctly, the above would be something we want to support, right?

@simonferquel
Copy link
Contributor Author

simonferquel commented Feb 12, 2019 via email

@simonferquel
Copy link
Contributor Author

Hmm, also, we must also be able to pass parameter values for such stateless actions like we do with docker app render -p nginx.scale=3. Definitely a tricky UX problem.

@radu-matei
Copy link
Member

Yeah, that totally makes sense, and I'll create a new issue to track this.

Could you please, also sign your commits?
Otherwise, this LGTM.

@simonferquel
Copy link
Contributor Author

They are signed with git commit -s. Do you require a PGP signature ?

@radu-matei
Copy link
Member

Yeah, we haven't enforced this yet, but as it's best practice, I think we should start to.

Copy link
Member

@radu-matei radu-matei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Let's keep the conversation about Duffle CLI support in #629.

Thanks for your contribution!

@radu-matei
Copy link
Member

Oh, there's a small conflict on this - once it's resolved, I think we can go ahead and merge this, keeping the discussion about the UX issue in #629.

Thanks!

@simonferquel
Copy link
Contributor Author

Oops, sorry, totally forgot about this one. Rebasing right now

@simonferquel
Copy link
Contributor Author

Now rebased, PTAL @radu-matei

@radu-matei radu-matei merged commit 178fe2d into cnabio:master Mar 14, 2019
@ghost ghost removed the review label Mar 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants