adding component selection for diff by bryanl · Pull Request #592 · ksonnet/ksonnet

bryanl · 2018-06-07T00:54:58Z

This change adds components for diffs. You can now run:

ks diff local:default remote:default -c <component name>

To diff a component in a module, specify the component with a module name. e.g component (for a module in the root component) or nested.component (for a module in nested/ under components)

In addition, module support throughout the ks was updated

Modules allow ksonnet users to separate their configurations under components.
Modules are dot separated paths that correlate to their location on the file system. e.g module a is in /app/components/a
Modules can be deeply nested. e.g components in /app/components/nested/deeply would be in module nested.deeply
Modules are now added as a label to kubernetes resources in ksonnet.io/component. This will allow the user to determine which component created a resource

Fixes #581

Signed-off-by: bryanl bryanliles@gmail.com

coveralls · 2018-06-07T01:04:10Z

Pull Request Test Coverage Report for Build 972

166 of 214 (77.57%) changed or added relevant lines in 15 files are covered.
12 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.08%) to 69.838%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/component/jsonnet.go	2	3	66.67%
pkg/component/manager.go	4	5	80.0%
pkg/component/yaml.go	2	3	66.67%
pkg/component/locator.go	4	5	80.0%
pkg/cluster/show.go	26	28	92.86%
pkg/component/create.go	10	12	83.33%
pkg/actions/import.go	3	5	60.0%
pkg/diff/diff.go	15	18	83.33%
pkg/actions/diff.go	6	10	60.0%
pkg/component/module.go	47	51	92.16%

Files with Coverage Reduction	New Missed Lines	%
pkg/component/create.go	12	64.1%

Totals
Change from base Build 961:	0.08%
Covered Lines:	9720
Relevant Lines:	13918

💛 - Coveralls

Support: * `.` * `dot.name` * `__leading` In support of ksonnet/ksonnet#592 Signed-off-by: bryanl <bryanliles@gmail.com>

Support: * `.` * `dot.name` * `__leading` * `9p` In support of ksonnet/ksonnet#592 Signed-off-by: bryanl <bryanliles@gmail.com>

This change adds components for diffs. You can now run: ``` ks diff local:default remote:default -c <component name> ``` To diff a component in a module, specify the component with a module name. e.g component (for a module in the root component) or nested.component (for a module in nested/ under components) In addition, module support throughout the ks was updated * Modules allow ksonnet users to separate their configurations under components. * Modules are dot separated paths that correlate to their location on the file system. e.g module a is in /app/components/a * Modules can be deeply nested. e.g components in /app/components/nested/deeply would be in module nested.deeply * Modules are now added as a label to kubernetes resources in `ksonnet.io/component`. This will allow the user to determine which component created a resource Signed-off-by: bryanl bryanliles@gmail.com

underrun · 2018-06-11T20:23:10Z

pkg/actions/diff.go

+			}
 		case strings.HasPrefix(t, "-"):
 			diffRemoveColor.Fprintln(&buf, t)
+			if err != nil {


line above this doesn't do _, err = like previous case - missed this?

underrun · 2018-06-11T20:33:12Z

pkg/actions/import.go

 	componentParams := params.Params{}

+	var moduleName string
 	switch i.module {


why keep this a switch when there's only one case? is a default-only switch a common golang idiom?

Because 0 values are useful in Go. It's the same thing as:

var moduleName if i.module != "" && i.module != "/" { moduleName = i.module }

It's not a default only switch. There is a case statement in there too.

Signed-off-by: bryanl <bryanliles@gmail.com>

shomron · 2018-06-11T20:01:55Z

pkg/actions/prototype_preview.go

 func bindPrototypeParams(p *prototype.Prototype) *pflag.FlagSet {
 	fs := pflag.NewFlagSet("preview", pflag.ContinueOnError)

+	fs.String("module", "", "Component module")


How do we avoid collisions with prototype parameters?

flagModule?

Is this possible?

We could detect the collision and alert the user.

shomron · 2018-06-11T20:08:59Z

pkg/actions/prototype_use.go

 		return errors.Errorf("Command has too many arguments (takes a prototype name and a component name)")
 	}

+	moduleName, err := flags.GetString("module")


flagModule?

shomron · 2018-06-11T20:20:59Z

pkg/cluster/object.go

+		Latest()
+
+	if len(components) > 0 {
+		selector := fmt.Sprintf("%s in (%s)", clustermetadata.LabelComponent, strings.Join(components, ","))


We should consider adding escaping/validation to the components if they can break the selector.

Will this hold up this PR?

Doesn't have to. I can open an issue to harden this.

shomron · 2018-06-11T20:31:03Z

pkg/cluster/show.go

+	objects := make([]*unstructured.Unstructured, len(apiObjects))
+	for i := range apiObjects {
+		obj := apiObjects[i]
+		objects[i] = obj.DeepCopy()


Do we need a deep copy just for sorting below?
Could simplify to:

objects := make([]*unstructured.Unstructured, len(apiObjects)) copy(objects, apiObjects) sortByKind(objects)

shomron · 2018-06-11T20:34:30Z

pkg/cluster/show.go

+		o1 := apiObjects[i]
+		o2 := apiObjects[j]
+
+		if o1.GroupVersionKind().Kind < o2.GroupVersionKind().Kind {


We could call GroupVersionKind() once per object.

shomron · 2018-06-11T20:39:41Z

pkg/component/create.go

@@ -167,7 +167,7 @@ func isValidName(name string) bool {
 }

 func namespaceComponent(name string) (string, string) {


Comments appreciated :)

shomron · 2018-06-11T20:40:49Z

pkg/component/jsonnet.go

 	}

-	if j.module == "/" {
+	if j.module == "/" || j.module == "" {


When is a root module "/" vs ""?

Is / correct here, or should it be filepath.Separator?

No, it is /

shomron · 2018-06-11T21:01:35Z

pkg/pipeline/pipeline.go

-
-	"github.com/ksonnet/ksonnet/pkg/util/k8s"
-	"github.com/ksonnet/ksonnet/pkg/util/strings"
+	gostrings "strings"


I'm going to say stdlib should win the contest of which package doesn't get aliased.

Why? I don't think there is a hard and fast rule.

General familiarity and principal of least surprise. But this is a matter of preference, and only a suggestion :)

shomron

No blockers. Thanks for putting this together!

redbaron · 2018-11-30T12:22:42Z

@bryanl , this change doesn't take into account that not all resources can have .metadata.labels, for instance List do not have it, opened #891

bryanl requested review from shomron and underrun June 7, 2018 00:54

bryanl force-pushed the 581-diff-component branch from 9dd8d4a to 5ce5696 Compare June 7, 2018 12:00

bryanl added a commit to bryanl/ksonnet-lib that referenced this pull request Jun 7, 2018

Add more valid field ids

c313141

Support: * `.` * `dot.name` * `__leading` In support of ksonnet/ksonnet#592 Signed-off-by: bryanl <bryanliles@gmail.com>

bryanl mentioned this pull request Jun 7, 2018

Add more valid field ids ksonnet/ksonnet-lib#145

Merged

bryanl added a commit to bryanl/ksonnet-lib that referenced this pull request Jun 7, 2018

Add more valid field ids

10e862b

Support: * `.` * `dot.name` * `__leading` * `9p` In support of ksonnet/ksonnet#592 Signed-off-by: bryanl <bryanliles@gmail.com>

bryanl added a commit to ksonnet/ksonnet-lib that referenced this pull request Jun 7, 2018

Add more valid field ids

dfcaa3d

Support: * `.` * `dot.name` * `__leading` * `9p` In support of ksonnet/ksonnet#592 Signed-off-by: bryanl <bryanliles@gmail.com>

bryanl force-pushed the 581-diff-component branch 2 times, most recently from 70ea4c9 to 1628276 Compare June 8, 2018 11:43

bryanl force-pushed the 581-diff-component branch from 1628276 to 1935184 Compare June 8, 2018 17:41

bryanl requested a review from a team June 8, 2018 21:50

underrun reviewed Jun 11, 2018

View reviewed changes

assign error before checking it

41a462a

Signed-off-by: bryanl <bryanliles@gmail.com>

shomron reviewed Jun 11, 2018

View reviewed changes

shomron approved these changes Jun 11, 2018

View reviewed changes

bryanl merged commit 4a2c2cd into ksonnet:master Jun 12, 2018

bryanl deleted the 581-diff-component branch June 12, 2018 00:41

		@@ -167,7 +167,7 @@ func isValidName(name string) bool {
		}

		func namespaceComponent(name string) (string, string) {

Conversation

bryanl commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 972

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shomron left a comment

Choose a reason for hiding this comment

Uh oh!

redbaron commented Nov 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bryanl commented Jun 7, 2018 •

edited

Loading

coveralls commented Jun 7, 2018 •

edited

Loading