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

GabrielBrascher · Dec 10, 2018

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 · Dec 12, 2018

@blueorangutan package

blueorangutan · Dec 12, 2018

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

blueorangutan · Dec 12, 2018

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

yadvr · Dec 12, 2018

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

yadvr · Jan 3, 2019

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

blueorangutan · Jan 3, 2019

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

blueorangutan · Jan 3, 2019

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

yadvr · Jan 3, 2019

@blueorangutan test

blueorangutan · Jan 3, 2019

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

GabrielBrascher · Jan 3, 2019

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

blueorangutan · Jan 4, 2019

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 · Jan 15, 2019

...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 · Jan 15, 2019

...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 · Jan 15, 2019

...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 · Jan 15, 2019

...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 · Jan 15, 2019

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

yadvr · May 27, 2019

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

GabrielBrascher · May 27, 2019

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

yadvr · Jun 24, 2019

ping @GabrielBrascher

GabrielBrascher · Jun 24, 2019

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 · Jan 3, 2020

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

GabrielBrascher · Jan 3, 2020

@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

yadvr 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

yadvr removed this from the 4.13.0.0 milestone May 27, 2019

yadvr 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

Search code, repositories, users, issues, pull requests...

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!

yadvr 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!

yadvr commented Jan 3, 2019

Uh oh!

blueorangutan commented Jan 3, 2019

Uh oh!

blueorangutan commented Jan 3, 2019

Uh oh!

yadvr 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!

yadvr commented May 27, 2019

Uh oh!

GabrielBrascher commented May 27, 2019

Uh oh!

yadvr 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!

6 participants

GabrielBrascher commented Dec 10, 2018 •

edited by rafaelweingartner

Loading