You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by geomacy <gi...@git.apache.org> on 2015/11/13 19:02:18 UTC
[GitHub] incubator-brooklyn pull request: Restore old HttpTool / Response f...
GitHub user geomacy opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/1032
Restore old HttpTool / Response for backward compatibility
PR follow-up to comment https://github.com/apache/incubator-brooklyn/pull/994#discussion_r44706399
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/geomacy/incubator-brooklyn keep-old-httptool-for-backward-compatibility
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/1032.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 #1032
----
commit 10b65ea6865041ea2fccb707a25375abfb93c2d9
Author: Geoff Macartney <ge...@cloudsoftcorp.com>
Date: 2015-11-13T17:30:44Z
Restore old copies of HttpTool/HttpResponse (but deprecated) so that 0.9.0 does not break
backward compatibility.
----
---
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] incubator-brooklyn pull request: Restore old HttpTool / Response f...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1032#issuecomment-157310766
As discussed on mailing list, I'm against. Short of introducing alternates for every call which references these classes, it's impossible to completely preserve backwards binary compatibility for these changes. The halfway house of leaving it as was (closing this) and putting a comment in the release notes seems the pragmatic choice.
---
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] incubator-brooklyn pull request: Restore old HttpTool / Response f...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1032#issuecomment-157391964
i.e. remove it altogether? Yes, I'll update the pull request accordingly.
---
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] incubator-brooklyn pull request: Restore old HttpTool / Response f...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1032#issuecomment-157316451
Ok, I shall leave this as-is then.
---
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] incubator-brooklyn pull request: Restore old HttpTool / Response f...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1032#issuecomment-157673865
I've updated the core tool and response classes in [14945f7](https://github.com/apache/incubator-brooklyn/commit/14945f75b1e0a4a5cc17dbb30459d092649bf770)
---
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] incubator-brooklyn pull request: Restore old HttpTool / Response f...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1032#issuecomment-157322569
No strong opinion on the backwards compatibility. What I'd like though is to either merge this or completely remove HttpTool in core (just leaving a shell class with a javadoc which points to the new one). This way we won't have an HttpTool in core, returning HttpToolResponse from utils.
---
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] incubator-brooklyn pull request: Restore old HttpTool / Response f...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1032#issuecomment-156550709
Thanks @geomacy, merging.
---
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] incubator-brooklyn pull request: Restore old HttpTool / Response f...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1032#issuecomment-157685119
Can you squash. Are you updating the release notes separately?
---
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] incubator-brooklyn pull request: Restore old HttpTool / Response f...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1032#issuecomment-157689774
Release notes updated and changes squashed.
---
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] incubator-brooklyn pull request: Restore old HttpTool / Response f...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1032#issuecomment-156574267
Will do that, thanks Aled.
G
On Fri, 13 Nov 2015 22:11 Aled Sage <no...@github.com> wrote:
> Thanks @geomacy <https://github.com/geomacy> - I'd also include a comment
> at the top of each of those classes to explain why the code is still there
> (rather than the "extends" mechanism). For example, copy-paste part of
> Svet's comment from #994 (comment)
> <https://github.com/apache/incubator-brooklyn/pull/994#discussion_r44706399>
> .
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/incubator-brooklyn/pull/1032#issuecomment-156573077>
> .
>
---
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] incubator-brooklyn pull request: Restore old HttpTool / Response f...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1032#issuecomment-156573077
Thanks @geomacy - I'd also include a comment at the top of each of those classes to explain why the code is still there (rather than the "extends" mechanism). For example, copy-paste part of Svet's comment from https://github.com/apache/incubator-brooklyn/pull/994#discussion_r44706399.
---
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] incubator-brooklyn pull request: Restore old HttpTool / Response f...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1032#issuecomment-157383449
@neykov fine with your plans to gut `HttpTool` core, rather than allow it to (surprisingly) return the new non-core `HttpToolResponse`
@geomacy can you do that?
---
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] incubator-brooklyn pull request: Restore old HttpTool / Response f...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1032#issuecomment-156557447
Did some more testing, unfortunately this doesn't fully solve the problem. Will hold off merging so we can further discuss it. The changes here are perfectly good though, but not enough.
There's an API that's heavilty used in entities - `HttpFeed` and related (i.e. `HttpValueFunction`, `HttpPollConfig`). The package renaming changes the signature of the methods in these classes so that already compiled entities against pre-changes version will not be compatible with post-changes versions. Probably worth moving to the dev list.
---
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] incubator-brooklyn pull request: Restore old HttpTool / Response f...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1032#issuecomment-157317157
Or rather, should I just close this pull request?
---
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] incubator-brooklyn pull request: Restore old HttpTool / Response f...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1032#issuecomment-157706690
LGTM, will merge.
---
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] incubator-brooklyn pull request: Restore old HttpTool / Response f...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1032#issuecomment-157320455
My suggestion is to close this. @neykov @aledsage you agree?
---
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] incubator-brooklyn pull request: Restore old HttpTool / Response f...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/1032
---
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.
---