Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/714#issuecomment-140379362
@wilderrodrigues I got your point, and I also hate when projects that I
work start breaking as a consequence of bad/poor code. Here goes an explanation
in a timeline manner, so you can get a better understanding of everything that
happened:
When I assigned that âtaskâ to an intern (@pedro-martins), he executed
it going class by class literally (I have to give credits for him to be so calm
and controlled, I would not stand to do that). While doing that he noted down
classes that he was changing the code. When he pushed, @DaanHoogland pointed
out that the PR was adding more lines then removing due to code formatting.
Therefore, he created a script that was fed with that list of classes and
performed the changes (deleted the lines that had to be deleted and changed the
variable names that had to be changed). Another problem arose, for some reason
in a few classes such as âVMwareGuruâ there are static methods that do not
need to be static and that use the âloggerâ variable, with our PR that had
to be changed. He was supposed to find each of the classes that had a problem
from that âComponentLifecycleBaseâ hierarchic and fix it. Sadly, he missed
one class out of 380. We indeed tried to build the whole pro
ject with a maven install, but it did not work at that time, there was a
problem with a class that we have not touched.
After that the PR 714 was merged by @DaanHoogland, and then reverted by
@karuturi.
When I noticed that problem, I got the task and created a script that
really builds the whole âComponentLifecycleBaseâ hierarchic and executes
the deletes and changes on variable names that are needed. Then I manually
check the classes that have some problem, around 8 classes that have static
methods that do not need to be static. I created a new PR #778 for that,
because we did not find a way to re-open the PR #714. The first time I pushed
to PR 778 I successfully did a maven install on ACS to check for compilation
problems that might be caused by our change. Then the time passed on (conflicts
appeared) and I did another two rebases, the first one I hit a problem with a
missing class on âcloud-plugin-network-vcsâ and the last one another
missing class on âcloud-framework-dbâ.
I had sent an email asking about the first missing class I found, but got
no answers.
Now to answer your first question, I indeed created a script, but just
after the PR 714 was reverted (we were not kidding about that). Moreover, the
pushes I did to PR 778 I made sure to execute a maven run to build the project
and see if everything was ok. Sadly I hit other problems with missing classes.
Sadly during PR 714, we had problems with Travis build timeouts at the same
time that we were working on PR 714, if Travis CI was working properly we would
have spotted the problem and the PR 714 would not need to be reverted.
@wilderrodrigues now your last point, we indeed have a production and test
environments running ACS 4.3.2 (both the same version because we are extending
ACS, everything we build is added into our 4.3.2, tested and then pushed to our
production environment), that is why we are not building the components changed
and testing it here. We did not see a reason to add this PR was not also added
to 4.3.2.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
