add server version to `ark version` output #1116

skriss · 2018-12-05T17:07:03Z

Signed-off-by: Steve Kriss [email protected]

Fixes #770

Would appreciate a quick review on the CRD definition before I proceed with the rest of the PR.

cc @heptio/ark-team

carlisia · 2018-12-05T17:41:35Z

This lgtm.

ncdc · 2018-12-05T17:42:25Z

pkg/apis/ark/v1/status_request.go

+	// by the StatusRequestController.
+	ProcessedTimestamp metav1.Time `json:"processedTimestamp"`
+
+	// ServerVersion is the version of the Ark binary that the


Suggested change

// ServerVersion is the version of the Ark binary that the

// ServerVersion is the Ark server version.

skriss · 2018-12-06T16:52:36Z

Would appreciate another look at this when folks have a chance now that I've implemented the server-side logic. I did a couple of things differently than our existing patterns and would like input on whether they're good changes:

rather than putting all logic directly into the controller, I kept the controller pretty minimal (just enqueueing and dequeuing items) and put the actual domain logic into a new package, pkg/statusrequest.
I added pkg/builder with a helper type for building StatusRequest objects for test purposes. it looks pretty similar to some of the builders that are currently in pkg/util/test (though I dropped the With prefix on every method, e.g. .Name(...) instead of .WithName(...) to keep things shorter. Ideally would like to move all builders out of pkg/util and into here.

Still need to do the client-side updates to ark version.

ncdc

Took a quick look. 👍

pkg/controller/status_request_controller.go

skriss · 2018-12-06T22:23:22Z

Added client code. The output currently looks something like:

Client:
        Version: master
        Git commit: v0.10.0-38-gfa40cc19
        Git tree state: dirty
Server:
        Version: v0.10.1

ncdc · 2018-12-06T22:25:45Z

Should we add a --client-only flag to prevent trying to talk to the server? kubectl has one.

skriss · 2018-12-06T22:27:21Z

I included one :)

ncdc · 2018-12-07T00:07:53Z

My bad!

…

On Thu, Dec 6, 2018 at 5:27 PM Steve Kriss ***@***.***> wrote: I included one :) — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#1116 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAABYiO_4c2wXSq4Fkmu4pJeBt7sHxQQks5u2ZnJgaJpZM4ZDKth> .

skriss · 2018-12-20T15:40:16Z

rebased

skriss · 2019-01-03T16:10:10Z

@carlisia @nrb @wwitzel3 @ncdc ready for a full review

carlisia

Looks good 👍

ncdc · 2019-01-04T17:13:49Z

pkg/apis/ark/v1/status_request.go

+
+// StatusRequest is a request to access current status information about
+// the Ark server.
+type StatusRequest struct {


Is it worth calling this ServerStatusRequest?

🤷‍♂️ I think I'm fine either way - strong feelings? Anyone else?

I like being more specific as opposed to less

ncdc · 2019-01-04T17:15:34Z

pkg/apis/ark/v1/status_request.go

+	Phase StatusRequestPhase `json:"phase"`
+
+	// ProcessedTimestamp is when the StatusRequest was processed
+	// by the StatusRequestController.


I would omit the controller detail - that's not something an end user would want or need to know about.

ncdc · 2019-01-04T17:19:03Z

pkg/builder/status_request.go

+limitations under the License.
+*/
+
+package builder


Do you anticipate we'll add more builders over time? If so, maybe this organization would make more sense?

pkg/builders/ statusrequest someothertype

and then in each package, we'd have a plain Builder type along with NewBuilder, e.g.

package statusrequest type Builder struct {} func NewBuilder() *Builder {}

We could even align on a common function name to spit out the underlying object - perhaps Build()?

Or if we're afraid that pkg/builders/backup would conflict with var backup *v1.Backup, we could suffix all builder packages with builder - pkg/builders/backupbuilder and then instead of saying backupbuilder.NewBuilder() it would just be backupbuilder.New().

I do expect we'd add more, yes; over time, I'd like this to replace the existing test obj helpers and be a little more succinct and standardized.

What's the motivation for having a subpackage per type? I'm not sure on that aspect yet.

+1 for renaming to Build() to get the obj

I was hoping to eliminate the duplication of "builder(s)" in builders.NewStatusRequestBuilder.

maybe it should just go into pkg/statusrequest, as a type Builder struct and NewBuilder() constructor? And over time as we relocate controller logic into separate packages, each of those can define their own?

That seems reasonable. Although we'll have problems with single-word types, such as backup and restore (trying to use as both a variable name and package name).

ncdc · 2019-01-04T17:23:08Z

pkg/cmd/version/version.go

 )

-func NewCommand() *cobra.Command {
+func NewCommand(f client.Factory) *cobra.Command {
+	timeout := time.Minute


I would default this to 3 to 5 seconds

ncdc · 2019-01-04T17:25:52Z

pkg/cmd/version/version.go

-			fmt.Printf("Version: %s\n", buildinfo.Version)
-			fmt.Printf("Git commit: %s\n", buildinfo.FormattedGitSHA())
+		Run: func(c *cobra.Command, args []string) {
+			fmt.Println("Client:")


I know we haven't done this much (if any) to date, but is this worth splitting out into its own function so we can unit test it?

OK, refactored & added a unit test for what I think is the important part. PTAL.

ncdc · 2019-01-04T17:36:27Z

pkg/cmd/version/version.go

+	}
+	defer client.StatusRequests(namespace).Delete(created.Name, nil)
+
+	listOptions := metav1.ListOptions{


We have this same logic in downloadrequest.Stream(). It might be nice in the future to extract this to a helper, with a custom handler function to call in the watch.Modified case.

ncdc · 2019-01-04T17:42:35Z

pkg/statusrequest/process.go

+	arkv1client "github.com/heptio/ark/pkg/generated/clientset/versioned/typed/ark/v1"
+)
+
+const ttl = time.Hour


1 minute is probably sufficient, especially if we change the default client-side timeout to 3 or 5 seconds.

ncdc · 2019-01-04T17:44:21Z

pkg/controller/status_request_controller.go

+	}
+
+	for _, req := range items {
+		if req.Status.Phase == arkv1api.StatusRequestPhaseProcessed {


Is it worth readding everything, not just processed ones?

skriss · 2019-01-04T21:25:54Z

addressed most of the comments in new commits - still thinking about unit testing of the CLI run func

pkg/cmd/version/version.go

nrb · 2019-01-09T20:39:04Z

Could you include a changelog entry?

carlisia

Lgtm 👍

skriss · 2019-01-23T16:47:14Z

@ncdc could you take another look here? your comments should all be addressed. will squash before merge.

pkg/apis/ark/v1/register.go

pkg/controller/server_status_request_controller.go

ncdc · 2019-01-23T17:39:31Z

@skriss did another pass.

skriss · 2019-01-23T18:34:05Z

Changes made. Anything else? Will do some retesting this aft as well.

ncdc · 2019-01-23T18:37:17Z

LGTM

skriss · 2019-01-23T19:49:49Z

tests LGTM.

Signed-off-by: Steve Kriss <[email protected]>

skriss · 2019-01-23T19:57:04Z

squished.

ncdc reviewed Dec 5, 2018

View reviewed changes

skriss force-pushed the status-crd branch from 8591c3d to 6753a92 Compare December 6, 2018 16:44

ncdc reviewed Dec 6, 2018

View reviewed changes

pkg/controller/status_request_controller.go Outdated Show resolved Hide resolved

skriss force-pushed the status-crd branch 3 times, most recently from fa40cc1 to 8dacd0e Compare December 6, 2018 22:21

skriss force-pushed the status-crd branch from 8dacd0e to 560395a Compare December 20, 2018 15:38

skriss changed the title ~~[WIP] add server version to ark version output~~ add server version to ark version output Dec 20, 2018

skriss requested review from carlisia, nrb and wwitzel3 January 3, 2019 16:09

carlisia approved these changes Jan 3, 2019

View reviewed changes

ncdc reviewed Jan 4, 2019

View reviewed changes

skriss force-pushed the status-crd branch from 560395a to c33053f Compare January 4, 2019 21:25

ncdc reviewed Jan 9, 2019

View reviewed changes

pkg/cmd/version/version.go Outdated Show resolved Hide resolved

ncdc reviewed Jan 9, 2019

View reviewed changes

pkg/cmd/version/version.go Outdated Show resolved Hide resolved

carlisia approved these changes Jan 11, 2019

View reviewed changes

ncdc reviewed Jan 23, 2019

View reviewed changes

pkg/apis/ark/v1/register.go Outdated Show resolved Hide resolved

ncdc reviewed Jan 23, 2019

View reviewed changes

pkg/controller/server_status_request_controller.go Show resolved Hide resolved

show server version in ark version output using ServerStatusRequest CRD

8a58b21

Signed-off-by: Steve Kriss <[email protected]>

skriss force-pushed the status-crd branch from d142e90 to 8a58b21 Compare January 23, 2019 19:56

ncdc merged commit f90b8f9 into vmware-tanzu:master Jan 23, 2019

skriss deleted the status-crd branch January 23, 2019 20:44

	// ServerVersion is the version of the Ark binary that the
	// ServerVersion is the Ark server version.

add server version to ark version output #1116

add server version to ark version output #1116

Conversation

skriss commented Dec 5, 2018

carlisia commented Dec 5, 2018

Choose a reason for hiding this comment

skriss commented Dec 6, 2018

ncdc left a comment

Choose a reason for hiding this comment

skriss commented Dec 6, 2018

ncdc commented Dec 6, 2018

skriss commented Dec 6, 2018

ncdc commented Dec 7, 2018 via email

skriss commented Dec 20, 2018

skriss commented Jan 3, 2019

carlisia 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

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

skriss commented Jan 4, 2019

nrb commented Jan 9, 2019

carlisia left a comment

Choose a reason for hiding this comment

skriss commented Jan 23, 2019

ncdc commented Jan 23, 2019

skriss commented Jan 23, 2019

ncdc commented Jan 23, 2019 via email • edited Loading

skriss commented Jan 23, 2019

skriss commented Jan 23, 2019

add server version to `ark version` output #1116

add server version to `ark version` output #1116

ncdc commented Jan 23, 2019 via email •

edited

Loading