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

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?

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

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

+ 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 · 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

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

 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

 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

- 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

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

yadvr · 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.

yadvr · 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 58a7a6aCompare December 11, 2018 16:12

Address reviewers from abandoned PRapache#1597.
fd3945b

GabrielBrascher force-pushed the address-abandoned-pr-1597 branch from 58a7a6a to fd3945bCompare 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

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!

yadvrDec 12, 2018

Choose a reason for hiding this comment

Uh oh!

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

dhlalukuJan 15, 2019

Choose a reason for hiding this comment

Uh oh!

dhlalukuJan 15, 2019

Choose a reason for hiding this comment

Uh oh!

dhlalukuJan 15, 2019

Choose a reason for hiding this comment

Uh oh!

dhlalukuJan 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