FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[CLOUDSTACK-10197] Update DisplayText of XenServer tools ISO entry in the database when it already exists by rafaelweingartner · Pull Request #2470 · apache/cloudstack · GitHub

/ cloudstack Public

[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
ConversationCommits (2)ChecksFiles changed
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 commented Feb 28, 2018

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?

rafaelweingartner added this to the 4.12 milestone Feb 28, 2018
rafaelweingartner self-assigned this Feb 28, 2018
rafaelweingartner 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 exists Feb 28, 2018
Update the displayText of XenServer ISO when it already exist in the DB
e0d869d 
Besides updating the ISO display text, I also created unit test cases for 'createXenServerToolsIsoEntryInDatabase' and 'getActualIsoTemplate' methods.
rafaelweingartner force-pushed the updateDisplayTextXstoolsVdi branch from c9d862a to 4a4a08c Compare February 28, 2018 15:59
khos2ow approved these changes Feb 28, 2018
View reviewed changes

khos2ow 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

khos2ow commented Feb 28, 2018

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 force-pushed the updateDisplayTextXstoolsVdi branch from 4a4a08c to 77d0912 Compare February 28, 2018 18:12

rafaelweingartner commented Feb 28, 2018

Copy link
Copy Markdown
Member Author

@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 force-pushed the updateDisplayTextXstoolsVdi branch from 77d0912 to 87cca11 Compare February 28, 2018 20:01
DaanHoogland approved these changes Mar 2, 2018
View reviewed changes

DaanHoogland 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

nice cleanup again @rafaelweingartner

rafaelweingartner commented Mar 3, 2018

Copy link
Copy Markdown
Member Author

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

DaanHoogland commented Mar 5, 2018

Copy link
Copy Markdown
Contributor

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

blueorangutan commented Mar 5, 2018

Copy link
Copy Markdown

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

DaanHoogland commented Mar 5, 2018

Copy link
Copy Markdown
Contributor

@rhtyd can you review this, please?

blueorangutan commented Mar 5, 2018

Copy link
Copy Markdown

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

rafaelweingartner commented Mar 5, 2018

Copy link
Copy Markdown
Member Author

@DaanHoogland no it does not ;)

DaanHoogland commented Mar 5, 2018

Copy link
Copy Markdown
Contributor

@blueorangutan test

blueorangutan commented Mar 5, 2018

Copy link
Copy Markdown

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

blueorangutan commented Mar 5, 2018

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:

Test Result Time (s) Test File
test_hostha_enable_ha_when_host_in_maintenance Error 1.03 test_hostha_kvm.py

rafaelweingartner commented Mar 5, 2018

Copy link
Copy Markdown
Member Author

@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 commented Mar 5, 2018

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 commented Mar 7, 2018

Copy link
Copy Markdown
Member Author

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

DaanHoogland commented Mar 8, 2018

Copy link
Copy Markdown
Contributor

@rafaelweingartner no it is fine

rafaelweingartner merged commit c3e5c09 into apache:master Mar 8, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

DaanHoogland DaanHoogland approved these changes
yadvr Awaiting requested review from yadvr
+1 more reviewer
khos2ow khos2ow approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

rafaelweingartner

Labels

type:improvement

Projects

None yet

Milestone

4.12.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL