You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by nitin-maharana <gi...@git.apache.org> on 2015/09/04 13:22:20 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-8805: Domains become inactive ...
GitHub user nitin-maharana opened a pull request:
https://github.com/apache/cloudstack/pull/775
CLOUDSTACK-8805: Domains become inactive automatically.
Handled the '%' case by replacing that with a literal character rather than a wildcard character.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nitin-maharana/cloudstack CloudStack-Nitin
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/775.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 #775
----
commit 046d4798fdb1b0d514f1eee15a4711ca8faca86d
Author: Nitin Kumar Maharana <ni...@citrix.com>
Date: 2015-09-04T11:28:51Z
CLOUDSTACK-8805: Domains become inactive automatically.
Handled the '%' case by replacing that with a literal character rather than a wildcard character.
----
---
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 #775: CLOUDSTACK-8805: Domains become inactive automaticall...
Posted by sateesh-chodapuneedi <gi...@git.apache.org>.
Github user sateesh-chodapuneedi commented on the issue:
https://github.com/apache/cloudstack/pull/775
LGTM for code. Looks like tests are successful too based on results published by @bvbharatk
@karuturi Should we consider this for merge OR re-run tests again as the previous tests were some time back in june, 2016?
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by nitin-maharana <gi...@git.apache.org>.
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-141015775
@DaanHoogland, @rafaelweingartner Yes I understand, I also think there should be test cases for a change because we don't know the entire logic. As there is not enough docs in java files. Any inputs from yourside on how to write the unit tests will be very helpful. 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: CLOUDSTACK-8805: Domains become inactive ...
Posted by rajesh-battala <gi...@git.apache.org>.
Github user rajesh-battala commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-212803920
I have gone through the code. LGTM.
without UT for this patch might not be an issue.
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-138383672
Tested on 4.3.2, and in that version this problem happens.
I would say that calling a domain “%” sees odd, but in some cases I guess that might be needed. I tested the solution you proposed with squirrel SQL client, and that did not work out.
Have you tested on your environment?
In my database I created a domain called % and a subdomain called test, being /%/ the path of the parent domain and /%/test/ the path of the subdomain. Your change would generate an SQL such as this one (if I try to forcefully delete the domain %):
select * from domain where path like "%replace('/%/','%','[%]')%"
I have not tested on my environment, but just by looking how the SQLs are generated and how the filter values are set (using prepared statements), that does not seem to work. I think the best way would be to use \, to escape the % character.
Something like this I think would work:
sc1.addAnd("path", SearchCriteria.Op.LIKE, "%" + StringUtils.replace(domainHandle.getPath(), "%", "\\%") + "%");
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-175665698
@rafaelweingartner @nitin-maharana @DaanHoogland guys, this is really old; can we have a re-review please?
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by nitin-maharana <gi...@git.apache.org>.
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-141687462
Thanks for the suggestion. yes, I will follow the same. I will make an another PR with the change and test file.
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-140910285
@rafaelweingartner we keep growing methods and not writing unit tests. Thus our code base is worsening in quality or at least in maintainability. However trivial we should keep refactorring and adding unit test in order to prevent things like "First the method is huge (around 100 lines), and second it is a method that performs database access (it builds "queries" to retrieve data). Maybe,...". I dont want to mis-quote you but am getting weary of all the reasons not to write unit tests. There is a lot of valid reasons and we should break through and focus on writing more (unit-)tests anyway (not discarding integration tests on the way). So as you say let's refactor the method and add the unit tests. even when trivial at the moment and discarding 90% of them along the way we will not regret putting in the effort.
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by nitin-maharana <gi...@git.apache.org>.
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-139152995
@rafaelweingartner 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: CLOUDSTACK-8805: Domains become inactive ...
Posted by nitin-maharana <gi...@git.apache.org>.
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-138810904
Yes, It can be possible. But I have not hard-coded this change, I followed the way sql queries are generated everywhere. This way of generating SQL query is followed in entire code. I think we have to come up with a generic way to resolve this vulnerability issue in entire 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 pull request: CLOUDSTACK-8805: Domains become inactive ...
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-216188309
@rajesh-battala please rebase against latest master and update on status of the PR
LGTM, cc @swill
tag:easypr
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-139524565
@nitin-maharana you changed a single line in a 102 line method. Though the code LGTM, I do not feel confident. Can you write unit tests or specify how this can be reproduced and/or proven?
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-138651375
If you tested, that is ok to me. I had not had the opportunity to build and test the ACS with that code in my environment. I just analyzed the code and checked how the SQL was generated, and executed that SQL using Squirrel SQL client. Perhaps as you pointed out that the select I tested did not work because of SQL client I used.
The code LGTM.
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-175667904
The question here is: will we write a test case for the change? If so, it has to be done, otherwise the code is ok.
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-141720864
@nitin-maharana, you do not need to open a new PR. You can continue working on this one, just do whatever is needed and commit to this branch.
---
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 #775: CLOUDSTACK-8805: Domains become inactive autom...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/775
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-143352749
@nitin-maharana, you have to create (write) a test case for that. I do not mean to be rude, but those SS do not prove much. You should write a test case and extract the code a mentioned to make the writing of the test case achievable. If you do not know how to write them, I can give you a hand.
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-141683980
To get this PR going, you could extract the code “"%" + "replace(" + domainHandle.getPath() + ", '%', '[%]')" + "%"” of line 357 to a method and then create a test case to that method. The test case would check if the ‘%’ as part of a domainPath is being properly handled either replacing it with \% (escaping with) or as you did using the replace function of the database.
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-141056052
in general: factor out a small chunk of the method into a private method or helper class. (commit 1)
then unit test that. (commit 2)
then apply your fix (commit three) bull's eye (hopefully)
In your more particular case @nitin-maharana , I think we don't have the test infrastructure to do it.
We need to create it. I am not burdonning you with this but if you want to: the replace call you make is in the sql domain and what we'd need is sql unit stuff.
I am starting to feel my wearyness is ungrounded but those feeling are always dangerous. A replacement of the replace call in java space could make it more (read easier) testable. it might have performance ramifications.
that all said; let's discus risk versus priority of this fix. Does anybody have ideas on this?
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by nitin-maharana <gi...@git.apache.org>.
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-139862762
@DaanHoogland I will write unit tests for this. 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: CLOUDSTACK-8805: Domains become inactive ...
Posted by nitin-maharana <gi...@git.apache.org>.
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-138648018
I tested on 4.5. I used mysql 5.6 client. It is working perfectly fine.
I think the squirrel client doesn't support the replace function or there may be some issue with the client side. Can you please specify what kind of issue you got when you forcibly deleted the '%' named domain. Is it deleting the sibling domains or is there any error in squirrel client?
I tried with your proposed solution and tested, that is also working perfectly fine.
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-140915157
@ DaanHoogland, I understand you, I just commented that to show to @nitin-maharana that “we” (I) understand that writing a test case to a method like that can be pretty hard, and that we are open to discuss and help with that.
I just wanted to show support, because I have seen in other open source projects people rejecting PRs, just because the guy that created the PR is not well-known in the community and he/she had not written test cases for huge methods in which the developer had just made a pretty small fix like @nitin-maharana is doing. Most of the time developers debug the code, looking for the crux of the problem he/she is having, find and fix it without knowing the whole idea of the method, especially when methods have generic names, perform a lot of tasks and do not have proper java docs.
For instance: when I read, “cleanupDomain”, I do not fully understand what the method is doing. Is it a periodical task that is intended to remove dirty from domains? Is it meant to remove resources that are no longer used by users of a domain?
I know that one of the tasks of that method is to remove/disable domains and their sub-domains, but to disable/remove something for me is different from cleaning something up.
Every time I review a code and see a clear opportunity to extract code and write test cases I suggest that, but when that does not happens, I do not feel comfortable asking for them and blocking a PR. That might not feel welcoming for a new contributor that is devoting some of his/her time for free in a project.
Bottom line is that we liked the job he has done and are here to help him with ideas in how to write at least one test case for the change he has made.
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-138872339
@nitin-maharana, when I said that the String concatenation there might make ACS vulnerable to SQL injection attacks, I did not mean that your PR did that. What I meant was that the code that is there, even before your PR, is making ACs vulnerable.
I just mentioned about the vulnerability here to register the thought, so others can also think about the problem that might exist.
I do not know much of ACS DAOs, but if in other places such as VM API (in which users have access), the same String concatenations are used that can be a huge problem.
Besides that the code LGTM.
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by nitin-maharana <gi...@git.apache.org>.
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-143041644
Hi. Here we have to access the database to see the behaviour of replace function. Because the replace function is evaluated in database server side. But in unit test we cannot access the database. We know the replace function will work properly if the parameters are sent correctly. So I have tested whether the parameters are sent correctly to the replace function while executing. I have done in two scenarios. Before the code change and after this commit. I have shared two screenshots of my test.
1. Behaviour before this commit
2. Behaviour after this commit
---
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 #775: CLOUDSTACK-8805: Domains become inactive automaticall...
Posted by nitin-maharana <gi...@git.apache.org>.
Github user nitin-maharana commented on the issue:
https://github.com/apache/cloudstack/pull/775
Any update on this. This is a very old PR pending for a long time. Rebased the branch with latest 4.9
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-138654400
I just realized one thing.
Domain admins can create subdomains, that concatenation of strings and SQLs would not make us vulnerable to SQL injections attacks?
---
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 #775: CLOUDSTACK-8805: Domains become inactive automaticall...
Posted by nitin-maharana <gi...@git.apache.org>.
Github user nitin-maharana commented on the issue:
https://github.com/apache/cloudstack/pull/775
Updated base branch to 4.9.
---
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 #775: CLOUDSTACK-8805: Domains become inactive automaticall...
Posted by nitin-maharana <gi...@git.apache.org>.
Github user nitin-maharana commented on the issue:
https://github.com/apache/cloudstack/pull/775
### ACS CI BVT Run
**Sumarry:**
Build Number 131
Hypervisor xenserver
NetworkType Advanced
Passed=73
Failed=0
Skipped=3
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
**Skipped tests:**
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
test_deploy_vgpu_enabled_vm
**Passed test suits:**
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_vpc_vpn.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_routers.py
test_reset_vm_on_reboot.py
test_snapshots.py
test_deploy_vms_with_varied_deploymentplanners.py
test_login.py
test_list_ids_parameter.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_volumes.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_vm_life_cycle.py
test_disk_offerings.py
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by nitin-maharana <gi...@git.apache.org>.
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-138650940
I am making one more pull request with your solution from a different branch.
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by nitin-maharana <gi...@git.apache.org>.
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-137928719
Yes, we can create domain name with special character.
Previously the '%' symbol was considered as a wild character. By which it was matching with all of its child domains as well as sibling domains, It shouldn't be the case. It shouldn't match with sibling domains(issue). Here it replaces the '%' symbol to a literal character. It only selects all child domains to be deleted.
I got this behavior in ACS 4.5
Thanks,
Nitin
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-140824594
@DaanHoogland and @nitin-maharana, I was thinking about this PR. I do not think that it is easy to write unit test for this.
First the method is huge (around 100 lines), and second it is a method that performs database access (it builds "queries" to retrive data). Maybe, if you extract the creation of sc1 from lines 356 – 357 and then test the search criteria that was created. But still, that might not seem a pretty test case.
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-137878960
Is there such case in which domains names have special characters like %?
The way you explained the problem it would generate an SQL like this:
Select * from domain where path like '%/domain1/subdomainWith%%'
However, that will not return all of the domains.
Now I do not have access to my ACS environment to test, but I can do that on Monday. Which version of ACS did you get that behavior?
---
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: CLOUDSTACK-8805: Domains become inactive ...
Posted by swill <gi...@git.apache.org>.
Github user swill commented on the pull request:
https://github.com/apache/cloudstack/pull/775#issuecomment-216333866
How should I test this? I think this is in a pretty good place pending some verification...
---
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.
---