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
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
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
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
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,
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
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
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
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
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
Github user nitin-maharana closed the pull request at:
https://github.com/apache/cloudstack/pull/790
---
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
Github user nitin-maharana commented on the pull request:
https://github.com/apache/cloudstack/pull/790#issuecomment-140999446
@remibergsma Yes this one should be closed. PR #775 does the same thing.
Thanks.
---
If your project is set up for it, you can reply to this email and have
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
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
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/790#issuecomment-140866855
Both are valid solutions for the very same problem, we would have to choose
between one of them. @nitin-maharana said that he tested with the other
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
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/790#issuecomment-140862396
@nitin-maharana I see also PR #775, can this be closed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
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
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
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 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
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
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.
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
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
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
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
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
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:
29 matches
Mail list logo
