Add Image relocation Map to translate invocation image #43
Conversation
action/action.go
Outdated
| @@ -26,16 +26,21 @@ const stateful = false | |||
| // - status | |||
| type Action interface { | |||
| // Run an action, and record the status in the given claim | |||
| Run(*claim.Claim, credentials.Set, io.Writer) error | |||
| Run(*claim.Claim, credentials.Set, bundle.ImageRelocationMap, io.Writer) error | |||
There was a problem hiding this comment.
I wonder if maybe this relocation map should be stored in the Claim too 🤔
There was a problem hiding this comment.
I thought about that as well and it was going to be my first approach bu I realized that the relocation map is a parameter similar to the creds file.
The current process for relocation would be:
duffle relocate that will move the images a new registry, this is also the step that generations the relocation map
duffle import that will create the claim(we need this at this point because there is not support for installing thick bundle)
duffle install when we pass in the registry map
The advantage I see here is that if it behaves like the creds we can maybe relocate and then upgrade an installation. If we save it in the claims and relocate the bundle the claims will be out of date and the new mapping file will have to be passed.
The downside here is that we will have to pass on all the action the mapping and we need the operator would have to store that file somewhere.
If we move it to claims we would only need to pass the mapping on import/install. The drawback is that on upgrade we would have to supply the mapping and update the claim as well.
Nevertheless I am open to the change, what do y'll think?
There was a problem hiding this comment.
I think I prefer having it in the claim, as for the user this relocation file is somehow an implementation detail.
I guess it would be simpler for the user if the import command creates the claim with the relocation inside, so the user won't have to persist it manually or to remember which relocation file is linked with which CNAB bundle.
And about the upgrade, I think that a user upgrading a CNAB package with another one relocated somewhere else is (or should) not be a common use case (it could be seen as installing a brand new bundle).
There was a problem hiding this comment.
As per our conversation I moved the relocation map into the claims
There was a problem hiding this comment.
That looks good. Thanks for doing that.
joaopapereira
force-pushed
the
duffle-issue-785
branch
2 times, most recently
from
b63fbd1 to
d93924d
Compare
claim/claim.go
Outdated
| } | ||
|
|
||
| // ValidName is a regular expression that indicates whether a name is a valid claim name. | ||
| var ValidName = regexp.MustCompile("^[a-zA-Z0-9_-]+$") | ||
|
|
||
| // New creates a new Claim initialized for an installation operation. | ||
| func New(name string) (*Claim, error) { | ||
| func New(name string, relocationMap bundle.ImageRelocationMap) (*Claim, error) { |
There was a problem hiding this comment.
I'm a bit on the fence for this, as adding the relocationMap here it is not homogeneous with other fields (like parameters or even bundle) which are not initialized in this New function.
What about removing it from here and let the user set the relocation map herself afterwards?
There was a problem hiding this comment.
I'm with @silvin-lubecki on this. Unless having a non-empty relocation map is mandatory, I would prefer to not require it during construction.
There was a problem hiding this comment.
I was unsure as well when I added it. From the side we only call New on install so I assume that we could piggy back on it. Nevertheless it is a better patter to do not have it there.
Just updated the PR and remove the map from the New method
There was a problem hiding this comment.
I would prefer removing the map from the constructor, but that's a nit (IMO). So I'll LGTM1 this PR, and let @silvin-lubecki do the honors of the second LGTM when he believes all is ready.
Thanks again!
If a bundle is relocated the location of the invocation image changes. When a new claim is created a image relocation map can be provided. This map is used to ensure the invocation image choosen to run the commands is the relocated.
joaopapereira
force-pushed
the
duffle-issue-785
branch
from
d93924d to
49c8ae2
Compare
This PR start the solution for the issue: cnabio/duffle#785