Skip to content

[CLOUDSTACK-10197] Update DisplayText of XenServer tools ISO entry in the database when it already exists - #2470

Merged
rafaelweingartner merged 2 commits into
apache:masterfrom
rafaelweingartner:updateDisplayTextXstoolsVdi
Mar 8, 2018
Merged

[CLOUDSTACK-10197] Update DisplayText of XenServer tools ISO entry in the database when it already exists#2470
rafaelweingartner merged 2 commits into
apache:masterfrom
rafaelweingartner:updateDisplayTextXstoolsVdi

Conversation

@rafaelweingartner

Copy link
Copy Markdown
Member

Changes here were suggestions made to #2365, but for some reason, the PR was merged without considering them. Both the update of the ‘displayText’ value and unit test cases were suggested as improvements in that PR.

After coding, I also when through those classes and removed unused variables.

@khos2ow can you take a look here?

@rafaelweingartnerrafaelweingartner added this to the 4.12 milestone Feb 28, 2018
@rafaelweingartnerrafaelweingartner self-assigned this Feb 28, 2018
@rafaelweingartnerrafaelweingartner changed the title Update DisplayText of XenServer tools ISO entry in the database when it already exists[CLOUDSTACK-10197] Update DisplayText of XenServer tools ISO entry in the database when it already existsFeb 28, 2018
Besides updating the ISO display text, I also created unit test cases for 'createXenServerToolsIsoEntryInDatabase' and 'getActualIsoTemplate' methods.
@rafaelweingartner
rafaelweingartnerforce-pushed the updateDisplayTextXstoolsVdi branch from c9d862a to 4a4a08cCompareFebruary 28, 2018 15:59

@khos2owkhos2ow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @rafaelweingartner, I just noticed I've never pushed the changes you requested on #2365 !

@khos2ow

Copy link
Copy Markdown
Contributor

@rafaelweingartner It seems you have some "trailing spaces" issues on XcpServerDiscovererTest.java and couple of variable name issues. The latter one is really annoying, because it forces to have ^_?[a-zA-Z0-9]*$ for final variables.

@rafaelweingartner
rafaelweingartnerforce-pushed the updateDisplayTextXstoolsVdi branch from 4a4a08c to 77d0912CompareFebruary 28, 2018 18:12
@rafaelweingartner

Copy link
Copy Markdown
MemberAuthor

@khos2ow thanks for the review.
And yes, those check-style rules are quite annoying. We need to discuss and change them to better fit the common practice nowadays.

@rafaelweingartner
rafaelweingartnerforce-pushed the updateDisplayTextXstoolsVdi branch from 77d0912 to 87cca11CompareFebruary 28, 2018 20:01

@DaanHooglandDaanHoogland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice cleanup again @rafaelweingartner

@rafaelweingartner

Copy link
Copy Markdown
MemberAuthor

What do you think @DaanHoogland, do wee need to wait for integration tests here?

@DaanHoogland

Copy link
Copy Markdown
Contributor

@rafaelweingartner doesn't hurt does it?
@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@DaanHoogland

Copy link
Copy Markdown
Contributor

@rhtyd can you review this, please?

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1750

@rafaelweingartner

Copy link
Copy Markdown
MemberAuthor

@DaanHoogland no it does not ;)

@DaanHoogland

Copy link
Copy Markdown
Contributor

@blueorangutan test

@blueorangutan

Copy link
Copy Markdown

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan

Copy link
Copy Markdown

Trillian test result (tid-2313)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 29032 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2470-t2313-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 66 look OK, 1 have error(s)
Only failed tests results shown below:

TestResultTime (s)Test File
test_hostha_enable_ha_when_host_in_maintenanceError1.03test_hostha_kvm.py

@rafaelweingartner

Copy link
Copy Markdown
MemberAuthor

@DaanHoogland is this "test_hostha_enable_ha_when_host_in_maintenance" an intermittent error? I have seen this on #2425, which is a very different PR.

@DaanHoogland

Copy link
Copy Markdown
Contributor

Yes @rafaelweingartner , I've seen this one a lot and seen it pass as well. Your code here is not related to that test afaik.
@rhtyd @PaulAngus can you give second and third opinions?

@rafaelweingartner

Copy link
Copy Markdown
MemberAuthor

@DaanHoogland this one seems to be ready. Would you like to wait for a third and fourth eyes here?

@DaanHoogland

Copy link
Copy Markdown
Contributor

@rafaelweingartner no it is fine

@rafaelweingartner
rafaelweingartner merged commit c3e5c09 into apache:masterMar 8, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@rafaelweingartner@khos2ow@DaanHoogland@blueorangutan