Extract StackConverter from the StackClient #1152
Conversation
vdemeester
force-pushed
the
extract-converter
branch
2 times, most recently
from
ef079f8
to
ec45725
Compare
It makes it easier to get the correct stack from a compose config struct without requiring the client (and thus talking to k8s API) Signed-off-by: Vincent Demeester <vincent@sbr.pm>
vdemeester
force-pushed
the
extract-converter
branch
from
ec45725
to
f2e6ee6
Compare
| _, err := NewStackConverter("v1alpha1") | ||
| assert.Check(t, is.ErrorContains(err, "stack version v1alpha1 unsupported")) | ||
|
|
||
| _, err = NewStackConverter("v1beta1") |
There was a problem hiding this comment.
nit: check the StackConverter returned is not nil?
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| corev1 "k8s.io/client-go/kubernetes/typed/core/v1" | ||
| "k8s.io/client-go/rest" | ||
| ) | ||
|
|
||
| // StackClient talks to a kubernetes compose component. | ||
| type StackClient interface { | ||
| CreateOrUpdate(s stack) error | ||
| StackConverter |
There was a problem hiding this comment.
Maybe we can really split the 2 interfaces and the 2 implementations? And manipulate 2 objects, one StackClient on one side, and one StackConverter on the other side? I know it will be a more complicated refactoring, so maybe in a follow-up?
There was a problem hiding this comment.
Good point :) I did it that way to not do too much changes but I do agree it would be even better.. Doable in this PR too 😉
|
|
||
| func v1beta1FromCompose(stderr io.Writer, name string, cfg *composetypes.Config) (Stack, error) { | ||
| cfg.Version = v1beta1.MaxComposeVersion | ||
| st, err := v1beta2FromCompose(stderr, name, cfg) |
There was a problem hiding this comment.
this is confusing; what's beta2 doing in a beta1 conversion?
There was a problem hiding this comment.
I just moved code, but mainly, Stack struct holds both the composefile (v1beta1 and the v1beta2 struct) 😅
There was a problem hiding this comment.
cc @silvin-lubecki because I kinda agree we shouldn't need to do that 👼
| return Stack{}, errors.Wrapf(err, "the compose yaml file is invalid with v%s", v1beta1.MaxComposeVersion) | ||
| } | ||
|
|
||
| st.ComposeFile = string(res) |
There was a problem hiding this comment.
wondering if there's consequences of not passing the original file content; is it to normalize the YAML? (if so, we may need some comments explaining)
vdemeester
force-pushed
the
extract-converter
branch
2 times, most recently
from
b28ebcc
to
f2e6ee6
Compare
|
Well, LGTM as it is 😉 (+ the tests to improve) |
|
(commenting here, to prevent the comment from being collapsed if code is updated). Discussing #1152 (comment) with @vdemeester; What it comes down to is that we try to replicate the swarm (client-side) handling of compose files as much as possible; this means that the actual version ( cli/kubernetes/compose/v1beta1/parsing.go Lines 3 to 4 in bac0d8f For example, if a compose-file is used that has The flow is as follows;
Do we need to put some information about that in the description of Note that we were also discussing a limitation in the swarm handling of docker-compose files; unlike command-line options, the compose-file does not label features as being supported by a specific Docker API version, so even if the conversion to the internal format succeeded, actually deploying may fail if the API version doesn't support a feature. Not really sure what the best approach for that would be; annotate options in the compose-file (likely doesn't make sense); perhaps validate the internal representation, and make a swarm-equivalent of |
It makes it easier to get the correct stack from a compose config
struct without requiring the client (and thus talking to k8s API)
Signed-off-by: Vincent Demeester vincent@sbr.pm