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

Address reviewers from abandoned PR #1597. by GabrielBrascher · Pull Request #3091 · apache/cloudstack · GitHub

/ cloudstack Public

Address reviewers from abandoned PR #1597. - #3091

Closed
GabrielBrascher wants to merge 1 commit into
apache:masterfrom
CLDIN:address-abandoned-pr-1597
ConversationCommits (1)ChecksFiles changed
Closed

Address reviewers from abandoned PR #1597.#3091
GabrielBrascher wants to merge 1 commit into
apache:masterfrom
CLDIN:address-abandoned-pr-1597

Conversation

GabrielBrascher commented Dec 10, 2018 •
edited by rafaelweingartner
Loading

Copy link
Copy Markdown
Member

Description

This PR addresses reviewers from PR #1597. If approved we can then close PR #1597.

Fixes: #1597

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Screenshots (if appropriate):

How Has This Been Tested?

GabrielBrascher added this to the 4.12.0.0 milestone Dec 10, 2018
GabrielBrascher self-assigned this Dec 10, 2018
rafaelweingartner reviewed Dec 10, 2018
View reviewed changes
Comment thread server/src/main/java/com/cloud/deploy/FirstFitPlanner.java Outdated
Comment thread server/src/main/java/com/cloud/deploy/FirstFitPlanner.java Outdated
Comment thread server/src/main/java/com/cloud/deploy/FirstFitPlanner.java
Comment thread ...console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyHttpHandlerHelper.java Outdated
Comment thread server/src/main/java/com/cloud/deploy/FirstFitPlanner.java Outdated
Comment thread ...console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyHttpHandlerHelper.java
Comment thread ...console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyHttpHandlerHelper.java
GabrielBrascher force-pushed the address-abandoned-pr-1597 branch 4 times, most recently from 8de4362 to 58a7a6a Compare December 11, 2018 16:12
GabrielBrascher force-pushed the address-abandoned-pr-1597 branch from 58a7a6a to fd3945b Compare December 11, 2018 18:39

rafaelweingartner commented Dec 12, 2018

Copy link
Copy Markdown
Member

@blueorangutan package

blueorangutan commented Dec 12, 2018

Copy link
Copy Markdown

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

blueorangutan commented Dec 12, 2018

Copy link
Copy Markdown

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

yadvr reviewed Dec 12, 2018
View reviewed changes
Comment thread ...console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyHttpHandlerHelper.java
private static final String TOKEN = "token";

public static Map<String, String> getQueryMap(String query) {
String[] params = query.split("&");

yadvr Dec 12, 2018

Copy link
Copy Markdown
Member

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

Is there a way to parse URL (possibly a get request) to get the key/value map of params?

GabrielBrascher Dec 13, 2018

Copy link
Copy Markdown
Member Author

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

Yes, there is, thanks for pointing that!
I will ping you when update it (also extracting some lines to methods, bringing some test cases, and documenting).

yadvr commented Jan 3, 2019

Copy link
Copy Markdown
Member

@GabrielBrascher you can now package and kick tests yourself to help you with your 4.12 RM work
@blueorangutan package

blueorangutan commented Jan 3, 2019

Copy link
Copy Markdown

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

blueorangutan commented Jan 3, 2019

Copy link
Copy Markdown

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

yadvr commented Jan 3, 2019

Copy link
Copy Markdown
Member

@blueorangutan test

blueorangutan commented Jan 3, 2019

Copy link
Copy Markdown

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

GabrielBrascher commented Jan 3, 2019

Copy link
Copy Markdown
Member Author

Thanks, @rhtyd! I am going to update this PR soon, addressing your review, just missing a few test cases.

blueorangutan commented Jan 4, 2019

Copy link
Copy Markdown

Trillian test result (tid-3296)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 23742 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3091-t3296-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_multipleips_per_nic.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 68 look OK, 2 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
test_nic_secondaryip_add_remove Error 32.91 test_multipleips_per_nic.py
test_04_rvpc_network_garbage_collector_nics Failure 500.25 test_vpc_redundant.py

dhlaluku reviewed Jan 15, 2019
View reviewed changes
Comment thread ...console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyHttpHandlerHelper.java
for (String param : params) {
String[] paramTokens = param.split("=");
String[] paramTokens = param.split(EQUALS);
if (paramTokens != null && paramTokens.length == 2) {

dhlaluku Jan 15, 2019

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

@GabrielBrascher can you change this line to this "If (ArrayUtils.isNotEmpty(paramTokens) && paramTokens.length == 2)"?

dhlaluku reviewed Jan 15, 2019
View reviewed changes
Comment thread ...console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyHttpHandlerHelper.java
public static Map<String, String> getQueryMap(String query) {
String[] params = query.split("&");
String[] params = query.split(AND);
Map<String, String> map = new HashMap<String, String>();

dhlaluku Jan 15, 2019

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

This variable should be named "tokenMap" or something similar to show that it should only store a token key-value item.

This will save computation by getting rid of the guardUserInput(map) call in Line #59

dhlaluku reviewed Jan 15, 2019
View reviewed changes
Comment thread ...console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyHttpHandlerHelper.java
String value = param.split("=")[1];
String name = paramTokens[0];
String value = paramTokens[1];
map.put(name, value);

dhlaluku Jan 15, 2019

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

I think another "if n(name.equalsIgnoreCase(TOKEN)" is needed here considering that the map should only contain a "TOKEN" key. and then remove "guardUserInput(map)" call in line 59

dhlaluku reviewed Jan 15, 2019
View reviewed changes
Comment thread ...console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyHttpHandlerHelper.java
s_logger.error("Unable to decode token");
s_logger.error("Unable to decode token due to null console proxy client param");
}
} else {

dhlaluku Jan 15, 2019

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

This else statement seems redundant and can go if the tokenMap only contains 1 TOKEN item

dhlaluku suggested changes Jan 15, 2019
View reviewed changes

dhlaluku 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

@GabrielBrascher overall nice code refactoring. I left some suggestions for further code refactors

GabrielBrascher commented Jan 15, 2019

Copy link
Copy Markdown
Member Author

Thanks, @dhlaluku! I appreciate the review and I will ping you when updating the code.

GabrielBrascher modified the milestones: 4.12.0.0, 5.0.0.0 Jan 30, 2019
yadvr removed this from the 4.13.0.0 milestone May 27, 2019

yadvr commented May 27, 2019

Copy link
Copy Markdown
Member

ping @GabrielBrascher is this PR abandoned or you still plan to submit changes?

GabrielBrascher commented May 27, 2019

Copy link
Copy Markdown
Member Author

@rhtyd thanks for pinging, I will take a look at this PR.

yadvr added this to the 4.13.0.0 milestone May 27, 2019

yadvr commented Jun 24, 2019

Copy link
Copy Markdown
Member

ping @GabrielBrascher

GabrielBrascher modified the milestones: 4.13.0.0, 4.14.0.0 Jun 24, 2019

GabrielBrascher commented Jun 24, 2019

Copy link
Copy Markdown
Member Author

Thanks for pointing this PR @rhtyd. I am removing the 4.13.0.0 milestone as it is not critical, I will get back on this one on the future.

andrijapanicsb commented Jan 3, 2020

Copy link
Copy Markdown
Contributor

ping @GabrielBrascher any plans with this one, or should we move it to 4.15 milestone?

GabrielBrascher commented Jan 3, 2020

Copy link
Copy Markdown
Member Author

@andrijapanicsb I will close this one for now. I shall get back to it when having some free time.

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

yadvr yadvr left review comments
rafaelweingartner rafaelweingartner approved these changes
fmaximus Awaiting requested review from fmaximus
nvazquez Awaiting requested review from nvazquez
+1 more reviewer
dhlaluku dhlaluku requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

GabrielBrascher

Labels

status:Has a PR

Projects

None yet

Milestone

4.14.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL