Address reviewers from abandoned PR #1597. #3091

GabrielBrascher · 2018-12-10T15:41:43Z

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?

server/src/main/java/com/cloud/deploy/FirstFitPlanner.java

...console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyHttpHandlerHelper.java

server/src/main/java/com/cloud/deploy/FirstFitPlanner.java

...console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyHttpHandlerHelper.java

rafaelweingartner · 2018-12-12T17:31:04Z

@blueorangutan package

blueorangutan · 2018-12-12T17:31:34Z

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

blueorangutan · 2018-12-12T17:51:31Z

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

rohityadavcloud · 2018-12-12T18:23:22Z

...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("&");


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

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

rohityadavcloud · 2019-01-03T15:14:55Z

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

blueorangutan · 2019-01-03T15:15:13Z

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

blueorangutan · 2019-01-03T15:35:31Z

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

rohityadavcloud · 2019-01-03T15:46:41Z

@blueorangutan test

blueorangutan · 2019-01-03T15:47:12Z

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

GabrielBrascher · 2019-01-03T16:13:08Z

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

blueorangutan · 2019-01-04T00:42:16Z

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 · 2019-01-15T17:18:35Z

...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) {


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

dhlaluku · 2019-01-15T17:37:46Z

...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>();


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 · 2019-01-15T17:41:18Z

...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);


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 · 2019-01-15T17:46:43Z

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


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

dhlaluku

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

GabrielBrascher · 2019-01-15T18:00:22Z

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

rohityadavcloud · 2019-05-27T12:31:21Z

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

GabrielBrascher · 2019-05-27T12:38:44Z

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

rohityadavcloud · 2019-06-24T09:38:56Z

ping @GabrielBrascher

GabrielBrascher · 2019-06-24T17:41:42Z

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 · 2020-01-03T15:07:24Z

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

GabrielBrascher · 2020-01-03T20:03:19Z

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

GabrielBrascher added the status:Has a PR label Dec 10, 2018

GabrielBrascher added this to the 4.12.0.0 milestone Dec 10, 2018

GabrielBrascher self-assigned this Dec 10, 2018

GabrielBrascher requested a review from fmaximus December 10, 2018 15:41

rafaelweingartner reviewed Dec 10, 2018

View reviewed changes

GabrielBrascher force-pushed the address-abandoned-pr-1597 branch 4 times, most recently from 8de4362 to 58a7a6a Compare December 11, 2018 16:12

Address reviewers from abandoned PR apache#1597.

fd3945b

GabrielBrascher force-pushed the address-abandoned-pr-1597 branch from 58a7a6a to fd3945b Compare December 11, 2018 18:39

rafaelweingartner approved these changes Dec 12, 2018

View reviewed changes

rafaelweingartner requested a review from nvazquez December 12, 2018 17:29

rohityadavcloud reviewed Dec 12, 2018

View reviewed changes

dhlaluku reviewed Jan 15, 2019

View reviewed changes

dhlaluku suggested changes Jan 15, 2019

View reviewed changes

GabrielBrascher modified the milestones: 4.12.0.0, 5.0.0.0 Jan 30, 2019

rohityadavcloud removed this from the 4.13.0.0 milestone May 27, 2019

rohityadavcloud added this to the 4.13.0.0 milestone May 27, 2019

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

GabrielBrascher closed this Jan 3, 2020

Address reviewers from abandoned PR #1597. #3091

Address reviewers from abandoned PR #1597. #3091

Uh oh!

Conversation

GabrielBrascher commented Dec 10, 2018 • edited by rafaelweingartner Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rafaelweingartner commented Dec 12, 2018

Uh oh!

blueorangutan commented Dec 12, 2018

Uh oh!

blueorangutan commented Dec 12, 2018

Uh oh!

rohityadavcloud Dec 12, 2018

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher Dec 13, 2018

Choose a reason for hiding this comment

Uh oh!

rohityadavcloud commented Jan 3, 2019

Uh oh!

blueorangutan commented Jan 3, 2019

Uh oh!

blueorangutan commented Jan 3, 2019

Uh oh!

rohityadavcloud commented Jan 3, 2019

Uh oh!

blueorangutan commented Jan 3, 2019

Uh oh!

GabrielBrascher commented Jan 3, 2019

Uh oh!

blueorangutan commented Jan 4, 2019

Uh oh!

dhlaluku Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

dhlaluku Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

dhlaluku Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

dhlaluku Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

dhlaluku left a comment

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher commented Jan 15, 2019

Uh oh!

rohityadavcloud commented May 27, 2019

Uh oh!

GabrielBrascher commented May 27, 2019

Uh oh!

rohityadavcloud commented Jun 24, 2019

Uh oh!

GabrielBrascher commented Jun 24, 2019

Uh oh!

andrijapanicsb commented Jan 3, 2020

Uh oh!

GabrielBrascher commented Jan 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

GabrielBrascher commented Dec 10, 2018 •

edited by rafaelweingartner

Loading