You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by ashwinswaroop <gi...@git.apache.org> on 2015/09/12 00:39:40 UTC
[GitHub] cloudstack pull request: This is the fix for the JIRA issue CLOUDS...
GitHub user ashwinswaroop opened a pull request:
https://github.com/apache/cloudstack/pull/810
This is the fix for the JIRA issue CLOUDSTACK-8817.
This is my first contribution to Apache CloudStack. The return values for endpoint and startpoint have now been changed to Integer instead of String. Please let me know if this is the only change that is required or if there are additional files which will be impacted by this change which require modification also.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ashwinswaroop/cloudstack feature_x
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/810.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 #810
----
commit 4d626c11bc4f029788ddc0e12340d8e880521f8b
Author: Ashwin Swaroop <as...@gmail.com>
Date: 2015-09-11T22:27:31Z
This is the fix for the JIRA issue CLOUDSTACK-8817. The return values for endpoint and startpoint are now Integer instead of 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 pull request: This is the fix for the JIRA issue CLOUDS...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/810#issuecomment-139921174
@ashwinswaroop,
Well, now I have no idea what to do. Let’s hope someone with more experience reply to this thread.
---
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: This is the fix for the JIRA issue CLOUDS...
Posted by ashwinswaroop <gi...@git.apache.org>.
Github user ashwinswaroop commented on the pull request:
https://github.com/apache/cloudstack/pull/810#issuecomment-139703362
Okay that makes sense. I went through the entire project and found only one caller of setStartPort and setEndPort which was from the relevant class(Firewall Response). There were others, but they were referring to a different class. The relevant caller took an argument of the form x.toString previously, but now since I changed the setter definition and the variable types to Integer, I removed the .toString part since it was already an Integer being converted to a String. There was no getter definition at all in FirewallResponse.java. Should I go ahead and commit? Also, in what way should I adjust the comments on my commit like you mentioned?
Thanks for the help!
---
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: This is the fix for the JIRA issue CLOUDS...
Posted by ashwinswaroop <gi...@git.apache.org>.
Github user ashwinswaroop commented on the pull request:
https://github.com/apache/cloudstack/pull/810#issuecomment-139697149
Sorry for not changing the setter. I'm using eclipse now and I can see that there is a FirewallResponse.java file in both cloud and cloudstack-api. Should I make the changes in both places or only in cloudstack-api like last time? Also I have noticed various callers to setStartPort and getStartPort in the project. Will they need to be changed also, or should I leave everything else as it is?
---
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: This is the fix for the JIRA issue CLOUDS...
Posted by ashwinswaroop <gi...@git.apache.org>.
Github user ashwinswaroop commented on the pull request:
https://github.com/apache/cloudstack/pull/810#issuecomment-139910486
@rafaelweingartner I committed and pushed the new changes. I also changed the commit comment like you requested.
---
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: This is the fix for the JIRA issue CLOUDS...
Posted by resmo <gi...@git.apache.org>.
Github user resmo commented on the pull request:
https://github.com/apache/cloudstack/pull/810#issuecomment-139913727
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: This is the fix for the JIRA issue CLOUDS...
Posted by ashwinswaroop <gi...@git.apache.org>.
Github user ashwinswaroop commented on the pull request:
https://github.com/apache/cloudstack/pull/810#issuecomment-139920903
@rafaelweingartner Okay so should I still squash the commits or leave it as it is since it says the pull request has been merged and closed? Also I received this message cloudstack-pull-rats #586 ABORTED. Is there anything I need to do?
---
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: This is the fix for the JIRA issue CLOUDS...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/810#issuecomment-139969167
builds were aborted probably because the PR was merged.
This PR is properly merged to master with below commits
* | | 120e1cc Merge pull request #810 from ashwinswaroop/feature_x 9 hours ago [Wido den Hollander]
|\ \ \
| * | | 221624d CLOUDSTACK-8817: listFirewallRules response JSON startport/endport not an int 11 hours ago [Ashwin Swaroop]
| * | | 4d626c1 This is the fix for the JIRA issue CLOUDSTACK-8817. The return values for endpoint and startpoint are now Integer instead of String. 2 days ago [Ashwin Swaroop]
|
---
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: This is the fix for the JIRA issue CLOUDS...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/810#issuecomment-139914742
Automatically merged?!?
I think it would be better to have merged, just after the squash
---
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: This is the fix for the JIRA issue CLOUDS...
Posted by ashwinswaroop <gi...@git.apache.org>.
Github user ashwinswaroop commented on the pull request:
https://github.com/apache/cloudstack/pull/810#issuecomment-139927888
@resmo Do you know the reason for the 2 aborted messages above? Is there anything I should do?
---
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: This is the fix for the JIRA issue CLOUDS...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/810#issuecomment-139913021
Now the change seems ok.
I would just suggest you to squash the two commits, and fix the commit message (the first line of the comment is used to create a “title” for the commit, when that first line is too big, it breaks the message when it is displayed in the web view).
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: This is the fix for the JIRA issue CLOUDS...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/810#issuecomment-139698329
@ashwinswaroop, no problem.
About the class duplicity, it just seems that there are two classes, but in reality there is only one. The question is that the project cloudstack-api is located inside the Cloudstack (it is a module). You should open the class that appear to be in cloudstack-api, this way the Eclipse IDE will properly compile and show the errors that the class might have.
Sure you will have to change every single point that uses those getters and setters, if you want to proceed with that proposal.
I just asked about the IDE, because when I saw that you just changed the parameters type, but not the setters and getters, it seemed like someone using VI to edit source files.
---
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: This is the fix for the JIRA issue CLOUDS...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/810#issuecomment-139684337
Interesting your proposal, but you forgot to change the respective getters and setters method.
Didn’t you use an IDE to make that change?
I would also recommend you to adjust the comments on your 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 pull request: This is the fix for the JIRA issue CLOUDS...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/810
---
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.
---