-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Remove HTTPS/SSL support in SAMP module #6201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
fd6412f to
86f6c7f
Compare
|
I can't comment on the code itself but I don't have any objection on the goal it is trying to accomplish. As for moving it, I originally had a thought similar to your Last but not least, does this PR close or fix the following? If so, please add the "fix ..." or "close ..." magic incantations in your original post: |
|
Maybe we don't need to move it, though what we could do is now make SAMP a top-level section in the docs and skip the astropy.vo top-level docs altogether. |
|
Removing the HTTPS-related code is fine by me. Regarding the namespace, I don't think it needs to be in the I will mention parenthetically that we are experimenting with HTTPS in SAMP in relation to some other requirements (web samp from an HTTPS page), and it's possible that it might be desirable to look again at this code at some point in the future, though that's not definite. However, I still think it makes sense to remove this code now, since it's not currently part of the SAMP standard, and it's causing problems. |
|
@mbtaylor - thank you for the comments! Regarding moving the package, it would actually be quite simple to move it and maintain backward compatibility, so I think it's something we should seriously consider. I agree with @mbtaylor that having it under the vo namespace is actually misleading since it can be used without any actual VO infrastructure. I quite like |
|
👍 to just having it be |
|
@eteq @taldcroft @kelle - as coordinators, any objections to moving |
…dard), remove support for HTTPS/SSL from astropy.vo.samp, since this is not meant to be supported and HTTPS/SSL support in astropy.vo.samp is currently broken with recent versions of Python.
86f6c7f to
61f9576
Compare
|
@astrofrog - I have no objections to your plan. |
SSL doens't currently work reliably in astropy.vo.samp, and as @mbtaylor pointed out in #6187 (comment), 'there are serious problems with mixing SAMP and HTTPS'. Therefore, I suggest that we simply remove all HTTPS/SSL-related code (without deprecation, since it's currently broken) which simplifies the astropy.vo.samp code significantly.
@mbtaylor - do you agree with this?
Since astropy.vo.samp is now the only package in astropy.vo, and since many users might like to use SAMP without realizing it's a VO standard (but not a VO server/client!), I would suggest (in a separate PR) moving the package to either
astropy.io.sampor simplyastropy.samp. Any opinions on this?Closes #2064
Closes #5460