[PATCH conductor] Integrating Mustache #2 (rev. 2)

Jozef Zigmund jzigmund at redhat.com
Fri May 18 11:51:23 UTC 2012


On Thursday 17 May 2012 16:07:00 Imre Farkas wrote:
> On 05/16/2012 04:04 PM, Jozef Zigmund wrote:
> > On Monday 14 May 2012 16:15:15 ifarkas at redhat.com wrote:
> >> This patchset contains the mustache templates for the 3 remaining jquery
> >> templates
> > 
> > NACK, as we discussed about found issues by me.
> > 
> > Here is the summary what you would fix:
> > 
> > 1. compress lines over 80
> > 2. write specs for added/edited methods of images_controller
> 
> The images controller code is tested in features/image.feature. The show
> action is currently uncommented and you can also see the note above
> which states that first we need to refactor the usage of VCR to fix it.
> There are other efforts to refactor our test suite, and this patchset is
> not intended to do that.
> 

it seems that we misunderstood, one thing is cucumber test, other thing is 
rspec tests. I'm just pointing out that code of added/edited methods are 
untested. We should increase rspec's cover of code, because now our 
image_controller are totally without test covering.

Of course if someone other is working on rspec covering of our code(what 
includes also image_controller), then you can skip this step, otherwise you 
should add the rspec examples, at least of added/edited methods.

> > 3. 'Push all' button in images#show is abled to click when some providers
> > are disabled .. for the last point maybe good to ask Angus(if it's ok or
> > not, to be 100% sure)
> 
> The visibility of Push all button is defined by BZ #799981. I think if
> you want the functionality to be changed you should first file a BZ for
> that, but probably the button makes more sense as it is. This patchset
> is not about changing functionality, but refactor what we currently have.
> 

Sounds reasonable

> > Otherwise it's great job and everything is working.
> 
> I am about to send an updated patchset that fixes the line limit. I also
> fixed the catalog checking in deployables_controller you mentioned on
> IRC. More discussion on the above are welcome!
> 

I wil review your revision of this patchset.

> Thanks for the review!
> Imre



More information about the aeolus-devel mailing list