Util dir refactor #649

estroz · 2018-10-19T23:48:31Z

Description of the change:

Create a top-level internal dir, with a util sub dir that will contain utility packages with specific designations, ex. internal/util/fileutil will only contain functions, types, etc. that operate on files.
Break up and remove commands/operator-sdk/cmd/cmdutil/util.go
Break up and move pkg/util/file_util.go to internal/util/fileutil
Move pkg/tlsutil to pkg/tls and pkg/util/k8sutil to pkg/k8sutil

Motivation for the change: General utility packages and files containing disparate data are best avoided, especially those exported such that users can access internal data. As per @shawn-hurley's concerns, the way the SDK handle's utility functions needs reassessment. This is an attempt at doing so, and is open to modification/being thrown out.

shawn-hurley · 2018-10-23T18:04:38Z

I would like to propose util/tlsutil -> k8s/tls or tls I know we think of them as utility functions but wouldn't it be nice to expose them as a full functional tls package for k8s?

I like the move to internal for our internal utilities with specific packages for each!

I have not gone through the PR in detail but I would like to 👍 the approach and make sure that enough people agree then do a more thorough review.

hasbro17 · 2018-10-23T18:17:29Z

Yeah pkg/tlsutil is supposed to be a util that we want users to use(after we add some documentation for it), so it should probably be more exposed under its own package as pkg/tls.

pkg/util is more for our own internal utils.

estroz · 2018-10-23T21:14:24Z

@hasbro17 if pkg/util is for our internal utils then I'll move k8sutil into internal/util. @shawn-hurley I'll move pkg/util/tlsutil -> pkg/tls.

estroz · 2018-10-23T23:04:23Z

Note: this is failing because dep tries to find the new internal/util/k8sutil directory in master but cannot. This issue is a symptom of dep not supporting PR refspecs. Once golang/dep#1658 is merged, the SDK e2e test code can inject PR refspecs into Gopkg.toml and solve issues such as this.

… and exported them

…onstants instead of hard-coded strings for dirs and filenames

test/e2e/memcached_test.go

hasbro17 · 2018-10-25T18:23:53Z

pkg/scaffold/cmd.go

@@ -42,7 +44,7 @@ import (
 	"{{ .Repo }}/pkg/apis"
 	"{{ .Repo }}/pkg/controller"

-	k8sutil "github.com/operator-framework/operator-sdk/pkg/util/k8sutil"
+	k8sutil "github.com/operator-framework/operator-sdk/pkg/k8s"


I think we can keep this as pkg/k8sutil. That's more descriptive than pkg/k8s imo.

Especially give that we're aliasing it as k8sutil.

AlexNPavel · 2018-10-25T18:41:28Z

test/e2e/memcached_test.go

+		// https://github.com/golang/dep/pull/1658
+		solveFailRe := regexp.MustCompile(`(?m)^[ \t]*Solving failure:.+github\.com/operator-framework/operator-sdk.+:$`)
+		if solveFailRe.Match(cmdOut) {
+			prSlug, ok := os.LookupEnv("TRAVIS_PULL_REQUEST_SLUG")


I don't think it's necessary to duplicate this code. Since this has to be run at some point anyway, I think we should just run the gopkg fix immediately after the new command in all cases instead of later in the code. In that case, the regex check only contains a t.Fatal and we don't have duplicate code.

hasbro17

LGTM

…e exported commands/operator-sdk/cmd/*: use internal/util packages and get rid of command/.../cmdutil pkg/util/*: move all pkg utilities into pkg/util

commands/.../add/*,new.go: update to 'internal/util' imports

…e issue

estroz added refactoring needs discussion labels Oct 19, 2018

estroz requested review from spahl, mhrivnak, joelanford, shawn-hurley, AlexNPavel, lilic and hasbro17 October 19, 2018 23:48

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 19, 2018

estroz force-pushed the util-refactor branch 4 times, most recently from 5b3b4dd to e44cb2e Compare October 22, 2018 17:41

AlexNPavel mentioned this pull request Oct 22, 2018

commands/.../cmdutil: deduplicate updateRoleForResource and support ClusterRole #646

Merged

estroz force-pushed the util-refactor branch from e44cb2e to c7d6b1d Compare October 22, 2018 19:34

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 22, 2018

estroz force-pushed the util-refactor branch 2 times, most recently from 96addfd to 37d640d Compare October 22, 2018 23:20

estroz changed the title ~~[WIP] Util dir refactor~~ Util dir refactor Oct 23, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 23, 2018

estroz added 3 commits October 24, 2018 13:31

pkg/scaffold/*: moved filename constants to respective scaffold files…

b8fbbbd

… and exported them

pkg/scaffold/*: exported directory names

1f2fa72

pkg/scaffold/ansible/*: update Ansible scaffolds with scaffold constants

2477da4

commands/operator-sdk/cmd/*,pkg/scaffold/constants.go: use scaffold c…

be5ae8d

…onstants instead of hard-coded strings for dirs and filenames

estroz force-pushed the util-refactor branch 2 times, most recently from 15b5715 to 622339f Compare October 24, 2018 20:54

hasbro17 reviewed Oct 24, 2018

View reviewed changes

test/e2e/memcached_test.go Outdated Show resolved Hide resolved

estroz force-pushed the util-refactor branch 4 times, most recently from 3c354e5 to d2f13df Compare October 24, 2018 22:14

hasbro17 reviewed Oct 25, 2018

View reviewed changes

AlexNPavel reviewed Oct 25, 2018

View reviewed changes

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 25, 2018

hasbro17 approved these changes Oct 25, 2018

View reviewed changes

estroz force-pushed the util-refactor branch 2 times, most recently from fa77761 to 3edece9 Compare October 25, 2018 21:46

AlexNPavel approved these changes Oct 25, 2018

View reviewed changes

estroz force-pushed the util-refactor branch from 3edece9 to ee4f08c Compare October 25, 2018 22:27

estroz mentioned this pull request Oct 25, 2018

Constants refactor #648

Closed

internal: moved file and project utilities that are not intended to b…

1f17426

…e exported commands/operator-sdk/cmd/*: use internal/util packages and get rid of command/.../cmdutil pkg/util/*: move all pkg utilities into pkg/util

estroz force-pushed the util-refactor branch 2 times, most recently from 06807f2 to 6e1be93 Compare October 25, 2018 23:22

estroz added 3 commits October 25, 2018 16:28

pkg/util/k8sutil -> pkg/k8sutil, pkg/tlsutil -> pkg/tls

df4aa3b

pkg/scaffold/role.go: moved UpdateRoleForResource here

8d90764

commands/.../add/*,new.go: update to 'internal/util' imports

test/e2e/memcached_test.go: early branch sub to fix 'dep ensure' solv…

62eb94b

…e issue

estroz force-pushed the util-refactor branch from 6e1be93 to 62eb94b Compare October 25, 2018 23:29

estroz merged commit 5a9f7d1 into operator-framework:master Oct 25, 2018

estroz deleted the util-refactor branch October 25, 2018 23:46

estroz restored the util-refactor branch October 26, 2018 18:55

estroz deleted the util-refactor branch October 26, 2018 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Util dir refactor #649

Util dir refactor #649

Uh oh!

estroz commented Oct 19, 2018 •

edited

Loading

Uh oh!

shawn-hurley commented Oct 23, 2018

Uh oh!

hasbro17 commented Oct 23, 2018

Uh oh!

estroz commented Oct 23, 2018 •

edited

Loading

Uh oh!

estroz commented Oct 23, 2018 •

edited

Loading

Uh oh!

Uh oh!

hasbro17 Oct 25, 2018

Uh oh!

hasbro17 Oct 25, 2018

Uh oh!

AlexNPavel Oct 25, 2018

Uh oh!

hasbro17 left a comment

Uh oh!

Uh oh!

Util dir refactor #649

Util dir refactor #649

Uh oh!

Conversation

estroz commented Oct 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shawn-hurley commented Oct 23, 2018

Uh oh!

hasbro17 commented Oct 23, 2018

Uh oh!

estroz commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estroz commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hasbro17 Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

hasbro17 Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

AlexNPavel Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

hasbro17 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

estroz commented Oct 19, 2018 •

edited

Loading

estroz commented Oct 23, 2018 •

edited

Loading

estroz commented Oct 23, 2018 •

edited

Loading