search

SeaCloudsEU / SeaCloudsPlatform

Seamless adaptive multi-cloud management of service-based applications
http://www.seaclouds-project.eu/
Apache License 2.0
18 stars 20 forks source link

Improve/second dam generator refactoring #274

Closed kiuby88 closed 8 years ago

kiuby88 commented 8 years ago

Big DamGenerator Refactoring.

PaaS support was included. JBDC relations for IaaS and PaaS were added. EndpointRelations for IaaS and PaaS were added.

TODO:

kiuby88 commented 8 years ago

@perezp @rosogon could you take a look? @MicheleGuerriero some little modifications were added to monitoringdamgenerator could you take a look please?

codecov-io commented 8 years ago

Current coverage is 40.93%

Merging #274 into master will increase coverage by +4.20% as of fb9e698

@@            master    #274   diff @@
======================================
  Files          329     361     +32
  Stmts        12064   13110   +1046
  Branches      1548    1645     +97
  Methods          0       0        
======================================
+ Hit           4432    5366    +934
+ Partial        449     433     -16
- Missed        7183    7311    +128

Review entire Coverage Diff as of fb9e698

Powered by Codecov. Updated on successful CI builds.

rosogon commented 8 years ago

@kiuby88 : Nice way to implement the generator. I would like to test it live.

andreaturli commented 8 years ago

I don't think this is a refactoring: 41 commits, 64 files changed are a completely new implementation, instead. I don't think we should merge such a huge PR at this stage of the project (March 30!) as it is too risky for the project itself. wdyt @SeaCloudsEU/developers ?

ahgittin commented 8 years ago

an impressively huge piece of work but as a consequence a proper review will take time and high risk merging without that

kiuby88 commented 8 years ago

Thanks toy four reply @andreaturli and @ahgittin. Probably refactoring it is not the best work in this case because DamGenerator was rewritten completly. In any case, only DamGenerator was modified the interface it is the same and the produced artifacts (DAM, ADP, AAM) it also is the same. Probably, I should try to open little PR in order to improve the DamGenerator step by step but I develop this PR during the Semana Santa and I decided not open little PRs during the Easter week. Probably it was not the best decision because it is not the best team working technique.

PR integration it is not my decision but SeaClouds did not work before this PR. We have tested the results on a testing environment and it works, in any case I agree that a lot of refactoring could be done and a lot of unit and integration tests should be added.

rosogon commented 8 years ago

In my opinion, it is a higher risk not merging this. It works for ATOS case study.

fradandria commented 8 years ago

I would agree with Roman. Do we have alternatives? This PR resolves multiple issue regarding the ATOS use case implementation. we finally made this end to end demo deploying the ATOS services.... @kiuby88 Could we make additional tests, before to accept the PR? in any case I do not think here we are discussing about the utility of the new code ... dont' we?

rosogon commented 8 years ago

We have been using this code several days. Sure there are things to fix, but it is much better than the current code. LGTM. Please, @kiuby88 , squash in more meaningful commits.

perezp commented 8 years ago

Once we have seen the desing+plan+deployment demo using the piece of code in the PR, I agree with merging it.

I also agree with @ahgittin . This PR would require a lot of time to be properly reviewed. On the one hand, I see that it is risky to merge it as it is now since it may contain bugs and create technical debt if the design of the solution is not good enough. On the other hand, this PR does not break any piece of code that were currently working. It is a step forward from the current situation.