CLOUDSTACK-8834: Fixed unable to download Template , when in multi zones#804
CLOUDSTACK-8834: Fixed unable to download Template , when in multi zones#804asfgit merged 1 commit intoapache:masterfrom
Conversation
We were listing image stores by zone id which was resulting in listing of only one image store If in that image store its download state is not DOWNLOADED then download template is failing
|
LGTM |
|
cloudstack-pull-rats #573 ABORTED |
|
the ticket (CLOUDSTACK-8834) mentions a screenshot, which is not there. the explanation give reproduction hints but is not completely clear because of this. The change seems trivial but is [part of a big method that warrants refactoring and (unit/integration)testing. |
|
cloudstack-pull-analysis #509 ABORTED |
|
Attached the screenshots to ticket. |
|
LGTM |
CLOUDSTACK-8834: Fixed unable to download Template , when in multi zonesWe were listing image stores by zone id which was resulting in listing of only one image store If in that image store its download state is not DOWNLOADED then download template is failing * pr/804: CLOUDSTACK-8834: Fixed unable to download Template , when in multi zones We were listing image stores by zone id which was resulting in listing of only one image store If in that image store its download state is not DOWNLOADED then download template is failing Signed-off-by: Wido den Hollander <wido@widodh.nl>
|
We got a 👎 from @DaanHoogland and still the PR was merged without any explanation from the author. Was it tested by anyone? ping @bhaisaab @borisroman @remibergsma @miguelaferreira Cheers, |
|
@wilderrodrigues Not sure if @DaanHoogland actually 👎 it, he said he wouldn't do that. It's confusing though ;-) |
|
@remibergsma indeed. Perhaps I have been through many untested PRs today and one more just got my brain in a fuzzy state. My argument remains as the PR lacks context and tests, although it got merged. Cheers, |
|
@wilderrodrigues -> @DaanHoogland Didn't -1, he actually said the opposite. The screenshots where added to provide context (question from daan). I tested them to confirm working order. After which I asked @wido to merge them. |
|
I think Daan did not say -1 or 👎 while unit tests are welcome, does not hurt to be merciful on one or few line changes if they indeed fix a bug |
|
Forgive me for my double negative in, "Be assured that I won't 👎 on this.", I did mean to say that I won't stop others merging it, as @bhaisaab explained. I didn't +1 it myself because of my lack of understanding. |
|
@bhaisaab I would say unit-tests are necessary to any software project this big, and with this many independent contributions. You putting the issue of asking for unit-tests, or not, as a matter of personal conduct was rather unfortunate. The people that take the time to write and run unit-tests, to review PRs and ask contributors to pay attention to such tests, don't do that out of mercilessness. They do it for the sake of the project and ultimate everyone that works with/in it. |
This is downloaded by
"""
git clone https://github.com/kanaka/noVNC.git
git checkout origin/stable/v0.6
"""
The last commit is
"""
commit 5b78c230f96b23129abc9a067ac9283fe3147c41
Refs: [HEAD], {origin/stable/v0.6}
Merge: e8986fa 4389e1f
Author: Solly Ross <directxman12+github@gmail.com>
AuthorDate: Wed Apr 12 11:43:12 2017 -0400
Commit: GitHub <noreply@github.com>
CommitDate: Wed Apr 12 11:43:12 2017 -0400
Merge pull request apache#804 from nbibler/backport-619
Backport apache#619 into stable/v0.6
include/input.js | 26 ++++++++++++--------------
1 files changed, 12 insertions(+), 14 deletions(-)
"""
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
…clouds' Alteração do formato do campo Time do formulário de Recurring Snapshot Closes apache#804 See merge request scclouds/scclouds!336
10 participants
We were listing image stores by zone id which was resulting in listing of only one image store
If in that image store its download state is not DOWNLOADED then download template is failing