Support k8s 1.8.0 #90

bryanl · 2017-11-30T21:42:34Z

Making ksonnet-lib Kubernetes 1.8.0 aware.

jessicayuen · 2017-12-01T20:56:08Z

ksonnet-gen/kubeversion/version.go

+// Beta returns the beta status of the version.
+func Beta(k8sVersion string) bool {
+	verData, ok := versions[k8sVersion]
+	if !ok {


We probably don't want to be erroring out on if someone supplies 1.8.1 or 1.7.5. Thoughts on how this should be handled? I think we should check for patch versions and as long as the minor version is supported, return true / false accordingly. Only error where minor versions aren't supported.

Likewise is 1.6 currently supported by ks? If yes, we should probably return false for anything less than the highest minor version.

I've updated it to return false if 1) it doesn't know the version, 2) the version beta flag is set to false.

Think we may need to do a little more work here because ks wouldn't be able to warn users on 1.8.1.

Maybe for now just add a check:

const ( maxVersion := "v.1.8." ) if strings.HasPrefix(k8sVersion, maxVersion) { return true } ... (insert other logic you have)

or something.

jessicayuen · 2017-12-01T21:06:04Z

ksonnet-gen/jsonnet/rewrite_test.go

+	"importstr": "\"importstr\"",
+	"in":        "\"in\"",
+	// TODO: this needs to be resolved
+	// "local":      "\"local\"",


Does this need to be resolved?

Not today. It was broken in master as well.

Modifies ksonnet-gen to support the new definition semantics added in in Kubernetes 1.8.0. Signed-off-by: bryanl <bryanliles@gmail.com>

jessicayuen

This LGTM.

hausdorff

Couple of suggestions that are non-blocking.

Ran errcheck to make sure introduction of error into signatures didn't inadvertently introduce places we're accidentally ignoring an error.

Also I'm in favor of moving to the return-an-error approach (rather than the panic approach that exists currently). Historically we used panic to try to fail quickly with a stack trace, since we didn't how the swagger schema was structured. Now we do, so there's fewer advantages to it.

hausdorff · 2017-12-01T21:57:09Z

ksonnet-gen/ksonnet/emit.go

@@ -32,6 +40,39 @@ func Emit(spec *kubespec.APISpec, ksonnetLibSHA, k8sSHA *string) ([]byte, []byte
 // Root.
 //-----------------------------------------------------------------------------

+func usesLegacySchema(version string) (bool, error) {


Do you think this is worth sourcing to a semver library? I think this whole function can be a couple lines of code in that case.

Not in this change set. We don't even have a way of managing dependencies and we are under a time crunch.

hausdorff · 2017-12-01T22:01:29Z

ksonnet-gen/ksonnet/emit.go

@@ -46,26 +87,59 @@ type root struct {

 	ksonnetLibSHA *string
 	k8sSHA        *string
+
+	isLegacySchema bool


Personally? I would make usesLegacySchema a method (e.g., root#isLegacySchema) which inspects spec.info.Version and returns true if the version works.

Rationale is, I think it's a little harder to maintain isLegacySchema as state, and since we don't care about perf here, I don't see a penalty to re-parsing and comparing versions any time we need to know whether it's a legacy schema. It also simplifies newRoot somewhat.

hausdorff · 2017-12-01T22:07:31Z

ksonnet-gen/ksonnet/emit.go

+	for defName, def := range spec.Definitions {
+		if def.IsCRD() {
+			crds[defName] = false
+			ref := def.Properties["Schema"]


I wrote this with a hardcoded string to confirm it works, but if I was writing it for prod, I'd handle this logic in the SchemaDefinition rather than relying on the emission routines to parse the schema definitions. Maybe worth replacing SchemaDefinition#IsCRD with something like SchemaDefinition#CRDSchemaName() (DefinitionName, bool) (or something) which would allow us to write the following (which I estimate to be clearer):

if crdDefName, isCRD := def.CRDSchemaName(); isCRD { crds[defName] = false crds[crdDefName] = false }

So, this function would retrieve the "Schema" member, perform the strings.TrimPrefix below, and return as a DefinitionName, true (or "", false if not found).

hausdorff · 2017-12-01T22:11:31Z

ksonnet-gen/ksonnet/emit.go

@@ -85,60 +159,83 @@ func (root *root) emit(m *indentWriter) {
 	m.indent()

 	// Emit in sorted order so that we can diff the output.
+	groups := root.groups.toSortedSlice()
+	var groupNames []string


Hmm, I don't see groupNames being used anywhere? Am I missing something?

hausdorff · 2017-12-01T22:14:34Z

ksonnet-gen/ksonnet/emit.go

+	for _, g := range groups {
+		groupNames = append(groupNames, string(g.name))
+	}
+
 	for _, group := range root.groups.toSortedSlice() {


We're calling root.groups.toSortedSlice() twice (once here and once on 162). This should be deterministic, so it seems like we might as well just re-use instead of sorting twice?

hausdorff · 2017-12-01T22:22:40Z

ksonnet-gen/ksonnet/emit.go

 }

-func (root *root) createAPIObject(
+func (r *root) createAPIObject(


Just FYI, renaming root -> r causes the linter to complain. Here and below.

hausdorff · 2017-12-01T22:24:02Z

ksonnet-gen/ksonnet/emit.go

+	dn, err := parsedName.Unparse(r.isLegacySchema)
+	if err != nil {
+		return nil, fmt.Errorf("unparse definition name: %v", err)
+	}


Since dn is only used in the error, you might want to move this into the error block. (Though this doesn't apply, in my estimation, below in functions like getAPIObjectHelper.)

hausdorff · 2017-12-01T22:28:56Z

ksonnet-gen/ksonnet/emit.go

 	if ok {
-		log.Panicf("Duplicate object kinds with name '%s'", parsedName.Unparse())
+		var names []string


I think I might be missing something, but names doesn't seem to be used?

hausdorff · 2017-12-01T22:39:31Z

ksonnet-gen/kubespec/new.go

+	}
+)
+
+type description struct {


A few things.

It seems like description#Validate is not used?

It seems like actually describeDefinition is really just doing what DefinitionName#Parse should be doing? Should we just move this code there?

If I'm missing something in (2) and this needs to stay in its own file, do you mind if we call this file something other than new.go (and also rename description to something else, since it's really actually a DefinitionName)? :) (I suspect this was a placeholder name.)

hausdorff

I'm marking "approve" since I don't see any bugs and we are under pressure to ship. I do hope we can come back and address these comments, though.

jessicayuen · 2017-12-01T22:51:41Z

Discussed offline with @bryanl -- we will pull these issues up as a group and discuss next week. He is OOO for now, so I'm proceeding with the merge.

bryanl force-pushed the support-1.8 branch 2 times, most recently from ca31a8d to 7001eb7 Compare December 1, 2017 20:15

bryanl self-assigned this Dec 1, 2017

bryanl requested a review from jessicayuen December 1, 2017 20:16

jessicayuen reviewed Dec 1, 2017

View reviewed changes

bryanl force-pushed the support-1.8 branch from 7001eb7 to 088fc02 Compare December 1, 2017 21:13

bryanl changed the title ~~[WIP] Support k8s 1.8.0~~ Support k8s 1.8.0 Dec 1, 2017

bryanl force-pushed the support-1.8 branch from 088fc02 to 107c130 Compare December 1, 2017 21:37

Add support for k8s 1.8.0

f11f209

Modifies ksonnet-gen to support the new definition semantics added in in Kubernetes 1.8.0. Signed-off-by: bryanl <bryanliles@gmail.com>

bryanl force-pushed the support-1.8 branch from 3624d65 to f11f209 Compare December 1, 2017 21:49

jessicayuen approved these changes Dec 1, 2017

View reviewed changes

hausdorff suggested changes Dec 1, 2017

View reviewed changes

hausdorff approved these changes Dec 1, 2017

View reviewed changes

jessicayuen merged commit e807702 into ksonnet:master Dec 1, 2017

hausdorff mentioned this pull request Dec 1, 2017

Pay down the code debt from #90 #93

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support k8s 1.8.0 #90

Support k8s 1.8.0 #90

bryanl commented Nov 30, 2017 •

edited

Loading

jessicayuen Dec 1, 2017

bryanl Dec 1, 2017

jessicayuen Dec 1, 2017 •

edited

Loading

jessicayuen Dec 1, 2017

bryanl Dec 1, 2017

jessicayuen left a comment

hausdorff left a comment

hausdorff Dec 1, 2017

bryanl Dec 1, 2017

hausdorff Dec 1, 2017

hausdorff Dec 1, 2017

hausdorff Dec 1, 2017

hausdorff Dec 1, 2017

hausdorff Dec 1, 2017

hausdorff Dec 1, 2017

hausdorff Dec 1, 2017

hausdorff Dec 1, 2017

hausdorff left a comment

jessicayuen commented Dec 1, 2017

Support k8s 1.8.0 #90

Support k8s 1.8.0 #90

Conversation

bryanl commented Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessicayuen Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessicayuen left a comment

Choose a reason for hiding this comment

hausdorff 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hausdorff left a comment

Choose a reason for hiding this comment

jessicayuen commented Dec 1, 2017

bryanl commented Nov 30, 2017 •

edited

Loading

jessicayuen Dec 1, 2017 •

edited

Loading