Skip to content

Conversation

@vvraskin
Copy link
Contributor

I wanted to familiarise myself with the cli repo and picked up this bug #313.

While working on it, I've realised some spots where unit tests would be welcome, so I added some based on Ginkgo, which I personally find pretty useful for TDD approach, general code coverage and documentation. What do you think guys, is this the right direction to go?
@rabbah @mdeuser @drcariel @houshengbo

The proposed change still has some nuances to be done properly: gradle deps for ginkgo and gomega, some duplication and probably a general clean up.

@rabbah
Copy link
Member

rabbah commented Jul 29, 2018

Vadim thank you for taking this and adding proper unit test - this will help us improve the cli’s enginnering. I’m all for it.

Copy link
Member

@dubee dubee left a comment

Choose a reason for hiding this comment

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

Merged the related Go client PR.

Copy link
Member

@dubee dubee left a comment

Choose a reason for hiding this comment

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

You will have to updated the pinned version of the Go client to pick up the changes. For reference see: 4a5a372.

@vvraskin vvraskin force-pushed the triggers branch 4 times, most recently from 0a3f7cf to 0ffa47c Compare August 19, 2018 11:09
* NOTE: this function will exit in case of a processing error since it indicates a problem parsing parameters.
*/
func createOrUpdate(fqn *QualifiedName, trigger *whisk.Trigger, overwrite bool) {
func createOrUpdate(Client *whisk.Client, fqn *QualifiedName, trigger *whisk.Trigger, overwrite bool) {
Copy link
Member

Choose a reason for hiding this comment

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

Now that Client is a parameter, can we delete the comment // TODO get rid of these global modifiers?

Copy link
Member

Choose a reason for hiding this comment

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

yes, now it's "local".

* NOTE: this function will exit in case of a processing error since it indicates a problem parsing parameters.
*/
func createOrUpdate(fqn *QualifiedName, trigger *whisk.Trigger, overwrite bool) {
func createOrUpdate(Client *whisk.Client, fqn *QualifiedName, trigger *whisk.Trigger, overwrite bool) {
Copy link
Member

Choose a reason for hiding this comment

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

yes, now it's "local".

@rabbah
Copy link
Member

rabbah commented Jan 26, 2019

@vvraskin @dubee let's try to get this in before it rots.

@dubee dubee merged commit 0e7e180 into apache:master Jan 29, 2019
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.

3 participants