Skip to content

[skip ci] Implement REST API handler to return the version #6154

Merged
zjs merged 1 commit intovmware:feature/vic-machine-servicefrom
zjs:topic/vic-machine-service-impl-version
Sep 14, 2017
Merged

[skip ci] Implement REST API handler to return the version #6154
zjs merged 1 commit intovmware:feature/vic-machine-servicefrom
zjs:topic/vic-machine-service-impl-version

Conversation

@zjs
Copy link
Member

@zjs zjs commented Aug 29, 2017

Resolves #6016.

Note: this change builds on #6132 and, once approved, I would like this to land as the third commit on feature/vic-machine-service.

// GET /container/version
api.GetVersionHandler = operations.GetVersionHandlerFunc(func(params operations.GetVersionParams) middleware.Responder {
return middleware.NotImplemented("operation .GetVersion has not yet been implemented")
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about returning the version directly here without version_get.go (assuming it works)?

api.GetVersionHandler = operations.GetVersionHandlerFunc(func(params operations.GetVersionParams) middleware.Responder {
        return operations.NewGetVersionOK().WithPayload(version.GetBuild().ShortVersion()) 
 })

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that would work fine (and is similar to how I implemented it initially), but I think there's a lot of value in having the APIs handled consistently.

If configure_vic_machine.go is simply a dispatcher to logic in other files, and never the place where handler logic is implemented, I think it's easier to reason about. (It also allows for consistent patterns around, for example, testing.)

@zjs zjs changed the base branch from master to feature/vic-machine-service September 7, 2017 20:56
@zjs zjs force-pushed the topic/vic-machine-service-impl-version branch from b235b15 to 761b376 Compare September 14, 2017 17:34
@zjs zjs requested a review from jzt September 14, 2017 18:14
}

// Cleanup returns either an initialized connection, or nil if it was never initialized
func (l *LazySessionInteractor) Cleanup() (SessionInteractor, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth rebasing - I seem to remember these changes in a different PR (@hickeng's) that I suspect has already been merged.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this picked up a bunch of random CLs from other people. Doing a force push seems to have cleared that up.

@zjs zjs force-pushed the topic/vic-machine-service-impl-version branch from 761b376 to 1a82eb6 Compare September 14, 2017 21:07
@zjs zjs force-pushed the topic/vic-machine-service-impl-version branch from 1a82eb6 to b981097 Compare September 14, 2017 21:17
@zjs zjs merged commit fa3a215 into vmware:feature/vic-machine-service Sep 14, 2017
@zjs zjs deleted the topic/vic-machine-service-impl-version branch September 14, 2017 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants