search

apache / openwhisk-client-go

Go client library for the Apache OpenWhisk platform
https://openwhisk.apache.org/
Apache License 2.0
35 stars 44 forks source link

Added extra columns to activation records summary rows #116

Closed larandersson closed 5 years ago

larandersson commented 5 years ago

Description

Added extra columns for output from 'wsk activation list command'. The output now looks as indicated by the screenshot below. screenshot from 2019-02-19 20-57-23

Compared to current output, the following columns have been added:

  1. Datetime. Date and time of the start of the activation in ISO format
  2. Kind. The kind of the action, taken from the annotations of the activation. If undefined, unknown is output.
  3. Start. If container was started warm or cold, as indicated by the initTime annotation in the activation record.
  4. Duration. The duration of the activation. Provided in an appropriate measure, such as ms (milliseconds) or s (seconds).
  5. Status. The outcome of the activation, as indicated by the statusCode attribute of the activation record and translated to a descriptive text.
  6. Entity. The fully qualified entity name is provided as //:

I have signed an ICLA.

mdeuser commented 5 years ago

very nice! perhaps also add new fields to the activation list --full output for consistency

larandersson commented 5 years ago

@mdeuser The added fields are already in the --full output, but in JSON format They were always there (in the API response), but were not used before.

mdeuser commented 5 years ago

@larandersson - i was thinking about the new Start column value. i don't think that is explicitly included in the --full output. but maybe that's ok since it's just some sugar on top of the actual data... although, in the case of a warm start, there's really no corresponding explicit value in the activation dump.

larandersson commented 5 years ago

@mdeuser You're right, the Start column is not in the --full output. But the client can use deduction to figure it out, if needed. So, maybe we don't need to include it in the JSON output as well?

rabbah commented 5 years ago

@mdeuser I'd like to get this PR in before the upcoming release I'm working on. Any objections?

larandersson commented 5 years ago

@dubee @rabbah I don't minding adding a unit test, if it makes sense. However, based on what kind of unit and integration tests already exist for this package, I don't see how adding a mock test would add any real value. Such a unit test would probably make more sense in the CLI repository.

If you want me to add a unit test in that repository, do you think it would be best to add a new one to https://github.com/apache/incubator-openwhisk-cli/tree/master/tests/src/test/scala/org/apache/openwhisk/core/cli/test, or add the test to the existing one here: https://github.com/apache/incubator-openwhisk-cli/blob/master/tests/src/test/scala/system/basic/WskCliActivationTests.scala?

So, with that being said, maybe this PR could be merged into the master branch and if needed I can open up a new PR against the CLI for the unit test?

rabbah commented 5 years ago

@larandersson I'd suggest unit tests for the affected method - no mocking of API calls necessary. I've started a very tiny suite of go unit tests for the CLI but not here.

rabbah commented 5 years ago

Looking over the existing tests, since none are affected by the changes one can argue there's already a test gap. I'm going to accept the PR as is and address the rest separately.

rabbah commented 5 years ago

@larandersson you'll need to bump the govendor git hash in the cli repo to pick up this change.