search

semaphoreci / cli

Semaphore 2.0 Command Line Interface
Apache License 2.0
25 stars 13 forks source link

Edit scheduler rules with project #151

Closed milica-nerlovic closed 5 years ago

shiroyasha commented 5 years ago

@milica-nerlovic please install https://github.com/fatih/vim-go. It will take care of indentation and also make life with Go smoother.

milica-nerlovic commented 5 years ago

Thanks, fixed indentation. I feel like :see_no_evil: though.

shiroyasha commented 5 years ago

Looks good. Add tests before you merge.

Tests for edit are difficult we can skip those, but tests for create and apply would be good.

shiroyasha commented 5 years ago

Here is one good example https://github.com/semaphoreci/cli/blob/master/cmd/create_test.go#L54-L85

milica-nerlovic commented 5 years ago

Do you mean like this create project test, but with schedulers? https://github.com/semaphoreci/cli/blob/master/cmd/create_test.go#L14

We haven't implemented adding a project with scheduler already specified in the backend, but I guess it would make sense to do so. :slightly_smiling_face: We assumed most schedulers will be added with edits/updates. :sweat_smile:

shiroyasha commented 5 years ago

:D Heh, I didn't know about that. Still, a test for update would be nice.

milica-nerlovic commented 5 years ago

The test for editing projects already existed: https://github.com/semaphoreci/cli/blob/master/cmd/edit_test.go#L147. I haven't found any other place where we test updates. I changed it to test not just if outgoing requests happened, but also the resulting model payload: https://github.com/semaphoreci/cli/pull/151/commits/18a4e3a52377e012f3fc715077de7d3c29ddcfcf, based on the edit notification test case, since this PR doesn't change any functionality, it just changes the payload and model definition.

Since I'm getting a bit confused here, if you have more feedback about test coverage, please specify which exact use cases are not and should be covered. :bowing_man:

shiroyasha commented 5 years ago

The tests you have added here https://github.com/semaphoreci/cli/pull/151/files#diff-422e761c9bb3ab8a27ebea45c2fd8f40R202 are sufficient. The only important part for me was to cover these new fields at least some tests.

Feel free to merge and create a new release.

milica-nerlovic commented 5 years ago

Got it. :+1: Thanks for the feedback. :bowing_woman: