Use the generic Serializer in libgitops, and fix some API machinery nits #273
Conversation
|
I see some additional baremetal files being added here. What's the plan for moving from BareMetal to ExistingInfra? |
…t other types of secrets
twelho
force-pushed
the
libgitops_serializer_v2
branch
from
0b6bcfe
to
c40e686
Compare
Signed-off-by: Dennis Marttinen <dennis@weave.works>
Signed-off-by: Dennis Marttinen <dennis@weave.works>
|
@palemtnrider this was written before the ExistingInfra* refactor, but I've now rebased it on top of master with #224 merged. It should begin passing tests again, @luxas you could have one more look through it to make sure the rebase moved over all your intended changes. |
| @@ -9,9 +9,8 @@ import ( | |||
| "github.com/spf13/cobra" | |||
| wks "github.com/weaveworks/wksctl/pkg/apis/wksprovider/controller/wksctl" | |||
| machineutil "github.com/weaveworks/wksctl/pkg/cluster/machine" | |||
| existinginfrav1 "github.com/weaveworks/wksctl/pkg/existinginfra/v1alpha3" | |||
| "github.com/weaveworks/wksctl/pkg/scheme" | |||
There was a problem hiding this comment.
Maybe for later, but I'd move this to either github.com/weaveworks/wksctl/api/scheme or github.com/weaveworks/wksctl/pkg/apis/existinginfra/scheme.
There was a problem hiding this comment.
The scheme pulls in both pkg/baremetalproviderspec/v1alpha1 and pkg/existinginfra/v1alpha3, so it's fine to keep it under pkg until we get pkg/apis re-organized.
| // In the future, if we wish to support other kinds of secrets than SealedSecrets, we | ||
| // can just change this to do .Decode(fr), and switch on the type | ||
| if err := scheme.Serializer.Decoder().DecodeInto(fr, ss); err != nil { | ||
| return nil, nil, "", nil, errors.Wrapf(err, "File %q does not contain a sealed secret, couldn't decode", secretFileName) |
There was a problem hiding this comment.
Is it possible this error is something simpler, like bad indentation?
There was a problem hiding this comment.
Yes, changed the error message to be more generic.
| } | ||
| func Parse(rc io.ReadCloser) (ml []*clusterv1.Machine, bl []*existinginfrav1.ExistingInfraMachine, err error) { | ||
| // Read from the ReadCloser YAML document-by-document | ||
| fr := serializer.NewYAMLFrameReader(rc) |
There was a problem hiding this comment.
It's no longer necessary to close the reader?
There was a problem hiding this comment.
The FrameReader will close the reader when all elements have been read (when DecodeAll is called). Due to a libgitops bug (weaveworks/libgitops#33) if decoding fails for some reason the reader is currently left open, that needs to be fixed over there (not a blocker for this PR).
pkg/utilities/manifest/manifest.go
Outdated
| var SchemeGroupVersion = schema.GroupVersion{Group: GroupName, Version: GroupVersion} | ||
| var SchemeBuilder = runtime.NewSchemeBuilder(addKnownTypes) | ||
| var AddToScheme = SchemeBuilder.AddToScheme | ||
| func WithNamespace(fileOrString, namespace string) ([]byte, error) { |
There was a problem hiding this comment.
Not changed in this PR, but fileOrString seems weird : it's only used as a filename in one place, but in several other places what is passed in is the result from ioutil.ReadAll(). Anyway it could easily be two functions.
There was a problem hiding this comment.
Changed to use io.ReadCloser in faf7480, this removes redundant conversions and enables quite smooth usage with both files and bytes.
Signed-off-by: Dennis Marttinen <dennis@weave.works>
…ithNamespace Signed-off-by: Dennis Marttinen <dennis@weave.works>
twelho
force-pushed
the
libgitops_serializer_v2
branch
from
d53c0be
to
faf7480
Compare
This is a duplicate of #257, just created as a branch on this project instead of on my personal fork in order to run end-to-end tests.