You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Ashadeepa <gi...@git.apache.org> on 2017/02/21 13:17:32 UTC
[GitHub] cloudstack pull request #1957: CLOUDSTACK-9748:VPN Users search functionalit...
GitHub user Ashadeepa opened a pull request:
https://github.com/apache/cloudstack/pull/1957
CLOUDSTACK-9748:VPN Users search functionality broken
VPN Users search functionality broken
If you try to search VPN users with it\u2019s user name, you will not be able to search.
Fixed the same.
Parent PR : https://github.com/apache/cloudstack/pull/1910
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Accelerite/cloudstack CLOUDSTACK-9748
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1957.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1957
----
commit 588ececd045c9175b33647375fd702e3e37f2126
Author: root <ashadeepa debnath>
Date: 2017-01-17T18:09:17Z
CLOUDSTACK-9748:VPN Users search functionality broken
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1957: CLOUDSTACK-9748:VPN Users search functionality broke...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1957
@Ashadeepa thanks for the information.
As a tip, try to avoid opening duplicated PRs, it gets hard for reviewers to track changes and discussions.
As @ustcweizhou suggested, you could have used a push force to push the new changes to the old PR branch.
I think @syed has asked you before; what is this `keyword` parameter? The docs are not clear about it; it just says \u201cList by keyword\u201d. This property comes from `org.apache.cloudstack.api.BaseListCmd`. This parameter was not been used before here; so I am assuming you are repurposing it, or at least giving a use to a property that was already there.
Looking at the code, I am I see that you use the keyword value to list any username that matches it. My question is the following: is there a way to document this use case? Otherwise, only a hand full of people will know its use. I have to tell you, reading the docs (https://cloudstack.apache.org/api/apidocs-4.9/apis/listVpnUsers.html), I would never try to use the `keyword` parameter as a filter to list all users that may match a given string.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1957: CLOUDSTACK-9748:VPN Users search functionality broke...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1957
Actually, my concerns are not regarding the response. Reading the name of the method one can for sure understand what it returns. My concern is that reading the parameter name `keyword` I really have no clue that this keyword is part of a username. The `keyword` could be used to list users of domains that may match it; the same can be applied for projects. Did you understand what I mean?
I was trying to discuss if we could find a better way to document it. As you also checked, there are other methods that this situation happens. However, I really do not believe this is a good standard to follow.
Would not something like the following work as well?
```
...
sb.and("id", sb.entity().getId(), SearchCriteria.Op.EQ);
sb.and("state", sb.entity().getState(), Op.IN);
sb.and().op("username", sb.entity().getUsername(), SearchCriteria.Op.EQ).or("username", "%" + sb.entity().getUsername() + "%" , SearchCriteria.Op.LIKE);
...
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1957: CLOUDSTACK-9748:VPN Users search functionalit...
Posted by ustcweizhou <gi...@git.apache.org>.
Github user ustcweizhou commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1957#discussion_r102209429
--- Diff: server/src/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java ---
@@ -621,6 +627,10 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
sc.setParameters("username", username);
}
+ if (keyword!= null) {
--- End diff --
it seems line 630 to 633 are not needed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1957: CLOUDSTACK-9748:VPN Users search functionality broke...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1957
Packaging result: \u2714centos6 \u2714centos7 \u2714debian. JID-549
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1957: CLOUDSTACK-9748:VPN Users search functionality broke...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1957
@Ashadeepa why do we have 2 PRs for the same problem?
It seems that one of them can be closed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1957: CLOUDSTACK-9748:VPN Users search functionality broke...
Posted by Ashadeepa <gi...@git.apache.org>.
Github user Ashadeepa commented on the issue:
https://github.com/apache/cloudstack/pull/1957
@rafaelweingartner : Regarding that force commit, will surely keep that in mind in future.
About keyword documentation, I had gone through several cloudstack apis (E.g. https://cloudstack.apache.org/api/apidocs-4.9/apis/listVirtualMachines.html, https://cloudstack.apache.org/api/apidocs-4.9/apis/listNetworks.html, https://cloudstack.apache.org/api/apidocs-4.9/apis/listZones.html, etc) , similar thing are being used. I do agree what you are suggesting. However, IMO, the API name "listVpnUser" gives an idea about the response we will get, i.e. listing the vpn user using a search keyword.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1957: CLOUDSTACK-9748:VPN Users search functionality broke...
Posted by SudharmaJain <gi...@git.apache.org>.
Github user SudharmaJain commented on the issue:
https://github.com/apache/cloudstack/pull/1957
LGTM on the code changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1957: CLOUDSTACK-9748:VPN Users search functionality broke...
Posted by Ashadeepa <gi...@git.apache.org>.
Github user Ashadeepa commented on the issue:
https://github.com/apache/cloudstack/pull/1957
@rafaelweingartner : This is due to the change in my remote urls. Closing the old PR https://github.com/apache/cloudstack/issues/1910.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1957: CLOUDSTACK-9748:VPN Users search functionality broke...
Posted by Ashadeepa <gi...@git.apache.org>.
Github user Ashadeepa commented on the issue:
https://github.com/apache/cloudstack/pull/1957
@ustcweizhou : My bad. Amended the changes . Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1957: CLOUDSTACK-9748:VPN Users search functionalit...
Posted by Ashadeepa <gi...@git.apache.org>.
Github user Ashadeepa commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1957#discussion_r102222411
--- Diff: server/src/com/cloud/network/vpn/RemoteAccessVpnManagerImpl.java ---
@@ -621,6 +627,10 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
sc.setParameters("username", username);
}
+ if (keyword!= null) {
--- End diff --
@ustcweizhou : My bad. Amended the changes . Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1957: CLOUDSTACK-9748:VPN Users search functionality broke...
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1957
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1957: CLOUDSTACK-9748:VPN Users search functionality broke...
Posted by ustcweizhou <gi...@git.apache.org>.
Github user ustcweizhou commented on the issue:
https://github.com/apache/cloudstack/pull/1957
tested. LGTM
btw, you can use "git push --force" to overwrite the code
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1957: CLOUDSTACK-9748:VPN Users search functionality broke...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1957
LGTM (code review).
@blueorangutan package
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---