Skip to content

[specific ci=Group23-VIC-Machine-Service] Format VCH creation log API response to text/plain#6640

Merged
AngieCris merged 1 commit intovmware:feature/vic-machine-servicefrom
AngieCris:creation-log-api-debug
Nov 8, 2017
Merged

[specific ci=Group23-VIC-Machine-Service] Format VCH creation log API response to text/plain#6640
AngieCris merged 1 commit intovmware:feature/vic-machine-servicefrom
AngieCris:creation-log-api-debug

Conversation

@AngieCris
Copy link
Contributor

@AngieCris AngieCris commented Oct 26, 2017

Fixes #6598

The VCH creation handler log output text/plain format (log messages upon 200 response code, and error string in all other HTTP error codes).

A snippet of how the handler response looks like upon 200:

Oct 10 2017 14:28:43.865-07:00 DEBUG Creating VMOMI session with thumbprint E6:57:47:95:CD:A4:BE:C9:10:9B:94:47:8D:57:59:07:EE:AA:F1:93
Oct 10 2017 14:28:44.300-07:00 DEBUG Session Environment Info: 
Oct 10 2017 14:28:44.300-07:00 DEBUG vSphere resource cache populating...
Oct 10 2017 14:28:44.410-07:00 DEBUG Cached dc: 
Oct 10 2017 14:28:44.577-07:00 DEBUG Cached cluster: 
Oct 10 2017 14:28:44.851-07:00 DEBUG Cached host: 
Oct 10 2017 14:28:45.028-07:00 DEBUG Cached pool: 
Oct 10 2017 14:28:45.080-07:00 DEBUG Cached folders: 
Oct 10 2017 14:28:45.080-07:00 DEBUG Error count populating vSphere cache: (1)
Oct 10 2017 14:28:45.080-07:00 DEBUG new validator Session.Populate: Failure finding ds (): default datastore resolves to multiple instances, please specify
Oct 10 2017 14:28:45.080-07:00 INFO  Validating target

Upon HTTP error codes:

"No log file available in datastore folder"

Integration tests are added to verify output containing log messages is multi-line, and error response is of string type.

@zjs zjs force-pushed the feature/vic-machine-service branch from 2faf0a5 to c98e4aa Compare October 27, 2017 19:55
@zjs
Copy link
Member

zjs commented Oct 30, 2017

I think this produces the right behavior (i.e., that the handler produces plain text responses), but I don't think this is the best implementation.

Currently, the swagger.json file specifies that this endpoint should produce a response of type "application/json" (as that is the value inherited, and not overridden, by the handler).

It seems like we should:

  1. Specify that this handler produces responses of type "text/plain".
  2. Rely on the auto-generated go-swagger code to return text. (I think redefining GetTargetTargetVchVchIDLogOK et. al is necessary only because go-swagger doesn't know we're trying to return text.)
  3. Pass error messages as strings (instead of error models) to the NewGetTargetTarget*VchVchIDLogDefault methods, if necessary for go-swagger to treat them as strings. (It'd be surprising for a consumer to call an API with an Accept header that indicates it's expecting a response of "text/plain" and get a response of "application/json" instead.)

I also wonder if we can expand the existing tests in the 23-05-VCH-Logs robot test suite to verify this change.

@zjs zjs force-pushed the feature/vic-machine-service branch from c98e4aa to e2957ac Compare October 31, 2017 16:43
@zjs zjs force-pushed the feature/vic-machine-service branch from e2957ac to 66411cf Compare October 31, 2017 23:19
@AngieCris AngieCris force-pushed the creation-log-api-debug branch from 8a225e3 to bad4f65 Compare November 1, 2017 17:46
@AngieCris AngieCris force-pushed the creation-log-api-debug branch from bad4f65 to 5f5faa4 Compare November 1, 2017 18:51
"responses": {
"200": { "$ref": "#/responses/vch-log" },
"default": { "$ref": "#/responses/error" }
"default": { "description":"Error when getting VCH logs", "schema": {"type": "string" } }
Copy link
Member

Choose a reason for hiding this comment

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

I think it might make sense to define a response for this (e.g., #/responses/error-plain) to avoid duplication between the two handlers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, just added the change

@zjs zjs force-pushed the feature/vic-machine-service branch 3 times, most recently from dfd6a3a to eb07b69 Compare November 6, 2017 21:23
@AngieCris AngieCris force-pushed the creation-log-api-debug branch from 8bdf2b4 to bb71564 Compare November 7, 2017 01:04
@zjs zjs force-pushed the feature/vic-machine-service branch 2 times, most recently from 2d7026a to 99035e4 Compare November 7, 2017 01:23
Copy link
Contributor

@mdharamadas1 mdharamadas1 left a comment

Choose a reason for hiding this comment

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

lgtm

@AngieCris AngieCris merged commit 5d4a742 into vmware:feature/vic-machine-service Nov 8, 2017
zjs pushed a commit that referenced this pull request Nov 15, 2017
error output is formatted to string as well.
zjs pushed a commit that referenced this pull request Nov 16, 2017
error output is formatted to string as well.
zjs pushed a commit that referenced this pull request Nov 20, 2017
error output is formatted to string as well.
AngieCris added a commit to AngieCris/vic that referenced this pull request Nov 20, 2017
error output is formatted to string as well.
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.

6 participants