Use the generic Serializer in libgitops, and fix some API machinery nits #257

luxas · 2020-07-02T13:59:11Z

This builds upon #251

This PR

stops using the client-go scheme as a stop-gap for registering types, and instead creates our own similar dropin-package for that with all types we need/know of.
removes unused code
Adds a register.go file for v1alpha1 (for being able to decode old configs later, when needed)
Uses the Serializer in libgitops for parsing. Later we might want to use the more sophisticated features of it like preserving comments support
Replaces custom JSON parsing with kyaml usage
Fixing problems uncovered in the code that weren't spotted before due to non-strict decoding, etc.

This fixes some of the nits highlighted in #172
cc @bboreham

… argument

bboreham

Excellent work, just a couple of notes and questions.

bboreham · 2020-07-06T16:58:39Z

pkg/kubernetes/drain/cordon.go

@@ -56,22 +53,6 @@ func NewCordonHelper(node *corev1.Node, desired DesiredCordonStatus) *CordonHelp
 	}
 }

-// NewCordonHelperFromRuntimeObject returns a new CordonHelper, or an error if given object is not a
-// node or cannot be encoded as JSON
-func NewCordonHelperFromRuntimeObject(nodeObject runtime.Object, scheme *runtime.Scheme, gvk schema.GroupVersionKind, desired DesiredCordonStatus) (*CordonHelper, error) {


Apparently this code was copied here due to difficulties importing the library created here so maybe we could revisit whether that can be done now.

Yes, I may open a dedicated PR to that refactor, as I'm not 100% sure how that drain code worked.

bboreham · 2020-07-06T17:01:44Z

pkg/cluster/machine/machine_test.go

+    private:
+      address: "172.17.8.101"


For the record this became an error after 8408805 and I missed the cleanup.

bboreham · 2020-07-06T17:04:13Z

pkg/cluster/machine/machine_test.go

-    authenticationWebhook:
-      cacheTTL: 2m0s
-      server:
-        url: http://127.0.0.1:5000/authenticate


These fields exist on BareMetalCluster so presumably some copy/paste error brought them here.

bboreham · 2020-07-06T17:07:19Z

pkg/specs/parse_test.go

+  infrastructureRef:
+    kind: BareMetalCluster
+    name: example


Are we really testing anything, if this was not noticed?

Now we are, as we're using strict parsing. Before, we did non-strict parsing, so all of these just resulted in unset fields.

Looking again, the example is named "clusterMissingBareMetalClusterDefinition" so probably valid to have this wrong.

bboreham · 2020-07-06T17:12:07Z

pkg/apis/wksprovider/machine/os/os.go

+	// Create the secret to decode into
+	ss := &ssv1alpha1.SealedSecret{}
+	// Decode the Sealed Secret into the object
+	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)


Previously the code was set up to handle multiple kinds of secret, although it only actually handled one.
I guess we can punt that to the time when we need a second kind, though it might be nice to note this in the commit description or maybe a comment.

…t other types of secrets

luxas · 2020-07-16T14:13:31Z

Failing at kubectl apply -f \\\"/tmp/03_secrets.yamlj9w2BC0l8O\\\" ) )'\"\nError from server (NotFound): error when creating \"/tmp/03_secrets.yamlj9w2BC0l8O\": namespaces \"weavek8sops\" not found

luxas · 2020-07-16T14:40:04Z

It was unexpected for me that the WithNamespace function also replaced the .metadata.name on Namespace kinds. Now should be fixed.

luxas · 2020-07-16T14:50:40Z

Hmm, next one:

level=error msg=Failed resource=\"kubectl:apply:cluster\"\nkubectl apply: failed to apply manifest /tmp/eS9GKlPCO4; output cluster.cluster.x-k8s.io/test-multimaster created\nThe BareMetalCluster \"test-multimaster\" is invalid: spec.sshKeyPath: Required value\n: command exited with 1
``

luxas · 2020-07-17T14:19:34Z

Closing in favor of: #273

luxas added 9 commits July 2, 2020 15:38

Add a register.go file to v1alpha1 types, and remove old deepcopy-gen…

84426da

… argument

Add a common scheme package

9eb482f

Remove unused NewScheme function and add comment to investigate

1b356dd

Remove unused file

aa6c762

Use our new scheme package in the controller

8bac622

Don't register any types into the client-go scheme

6d21f42

Remove unused code and use our scheme instead of client-go

64d5147

Use the libgitops serializer

e493033

Add libgitops import to go.mod

0623e8e

luxas assigned bboreham and twelho Jul 2, 2020

luxas added 2 commits July 2, 2020 18:17

fix problems in unit tests, e.g. YAML indentatation errors

f249e71

remove unused function

f85feed

bboreham reviewed Jul 6, 2020

View reviewed changes

luxas added 7 commits July 16, 2020 13:52

Merge branch 'master' into libgitops_serializer_v2

d0ca2be

Rewrite the manifest parsing to use kyaml & libgitops

05eaf54

update references to WithManifest

75842a3

Use WriteFrameList instead of yaml.Concat

0e544ba

go mod tidy

dc5d37a

Minor review comments. Use the serializer-embedded codecs and documen…

64a17a7

…t other types of secrets

add double quotes to test data, as output is in JSON

bfcf2a7

Special handling for namespaces

3987e9e

luxas added 3 commits July 16, 2020 18:01

Add quotes to the JSON matching test code

2d2b7ff

Make .spec.sshKeyPath optional

c906fbc

remove unnecessary quotes

10908cd

luxas force-pushed the libgitops_serializer_v2 branch from 6d913c6 to 10908cd Compare July 16, 2020 15:17

Fix YAML indentation

bac4bdf

luxas added 2 commits July 17, 2020 10:58

Maybe now this is actually building YAML manifests...

34cd206

YAML indentation, part 1000

0b6bcfe

luxas mentioned this pull request Jul 17, 2020

Use the generic Serializer in libgitops, and fix some API machinery nits #273

Merged

luxas closed this Jul 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the generic Serializer in libgitops, and fix some API machinery nits #257

Use the generic Serializer in libgitops, and fix some API machinery nits #257

luxas commented Jul 2, 2020 •

edited

Loading

bboreham left a comment

bboreham Jul 6, 2020

luxas Jul 16, 2020

bboreham Jul 6, 2020

bboreham Jul 6, 2020

bboreham Jul 6, 2020

luxas Jul 16, 2020

bboreham Jul 17, 2020

bboreham Jul 6, 2020

luxas commented Jul 16, 2020

luxas commented Jul 16, 2020

luxas commented Jul 16, 2020

luxas commented Jul 17, 2020

Use the generic Serializer in libgitops, and fix some API machinery nits #257

Use the generic Serializer in libgitops, and fix some API machinery nits #257

Conversation

luxas commented Jul 2, 2020 • edited Loading

bboreham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luxas commented Jul 16, 2020

luxas commented Jul 16, 2020

luxas commented Jul 16, 2020

luxas commented Jul 17, 2020

luxas commented Jul 2, 2020 •

edited

Loading