Switch CASDA to QueryWithLogin #2386

jd-au · 2022-04-30T06:04:51Z

This replaces the custom login in CASDA with use of QueryWithLogin.

pep8speaks · 2022-04-30T06:04:54Z

Hello @jd-au! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-05-06 00:38:43 UTC

codecov · 2022-04-30T06:34:17Z

Codecov Report

Merging #2386 (740b05a) into main (f20c85b) will decrease coverage by 0.04%.
The diff coverage is 46.15%.

@@            Coverage Diff             @@
##             main    #2386      +/-   ##
==========================================
- Coverage   62.87%   62.83%   -0.05%     
==========================================
  Files         133      133              
  Lines       17246    17264      +18     
==========================================
+ Hits        10844    10848       +4     
- Misses       6402     6416      +14

Impacted Files	Coverage Δ
astroquery/casda/core.py	`87.85% <46.15%> (-5.16%)`	⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

jd-au · 2022-04-30T06:37:54Z

astroquery/casda/tests/test_casda_remote.py

-                               'and CASDA_PASSWD environment variables)')
-    def test_stage_data(self):
+                               'and CASDA_PASSWD environment variables or password in keyring)')
+    def test_stage_data(self, cached_credentials):


I've shown two approaches to credentials for the remote tests. However there is no keyring backend on the CI server, so I am thinking of removing this approach based around the keyring.

If there is any "test" user that we could use, I'm happy to set up github tokens that the CI will have access to. But that definitely seems to be something beyond this PR, so any approach that works for now sounds good on my end (e.g. you have real login credentials and say that these tests pass, I'm happy with it).

I've run these tests with real credentials and they pass. These remote tests now require the keyring so they can't be run on the github CI environment.

bsipocz · 2022-05-02T19:02:32Z

astroquery/casda/core.py

        return self._complete_job(job_url, verbose)

-    def cutout(self, table, *, coordinates=None, radius=None, height=None,
+    def cutout(self, table, *, coordinates=None, radius=1*u.arcmin, height=None,


I think this was accidentally pulled in with one of the merge commits, please rebase to get rid of those and any unrelated changes.

The commit was intentional - just cleaning up a minor aspect that was missed in the last PR

bsipocz · 2022-05-02T19:09:11Z

astroquery/casda/tests/test_casda_remote.py

-                               'and CASDA_PASSWD environment variables)')
-    def test_stage_data(self):
+                               'and CASDA_PASSWD environment variables or password in keyring)')
+    def test_stage_data(self, cached_credentials):


If there is any "test" user that we could use, I'm happy to set up github tokens that the CI will have access to. But that definitely seems to be something beyond this PR, so any approach that works for now sounds good on my end (e.g. you have real login credentials and say that these tests pass, I'm happy with it).

bsipocz · 2022-05-02T19:25:33Z

astroquery/casda/core.py

-            self._authenticated = False
+
+    def _login(self, *, username=None, store_password=False,
+               reenter_password=False, password=None):


I'm a bit torn here as we're not at all consistent in the library as a whole, but having the password in the signature seems a bit of a counter-effective of security. I think I best like how the alma module is set up in this regard. What do you all think, am I just overly cautious?

@keflavich @andamian @pllim

I added the password param to allow testing of the login as there is no keyring backend in the CI environment. I couldn't see anyone else testing the login path. I agree the password param should not be used by other users, so perhaps either a "For testing use only" note in the method's docstring, or hiding it in a **kwargs would be better?

👎 to explicit password argument in API. Might as well set up the backend to handle anonymous login without a password, or enable a more secure way (encrypted token) for the CI. Or just not test this at all in the CI and set up something private server-side to test this securely.

Backend changes are out of scope for this piece of work unfortunately. I've removed the password parameter and the tests for the login code. For the other test code which requires login I have added a utility to simulate the login result.

Sounds good to me. Thanks!

Thank you. I opened #2391 with the aim of tracking this password in API issue for the other affected modules, and finding an acceptable backend solution for testing (very much beyond the scope for this PR) has already have one opened one in #2367.

A consequence of removing the tests relying on the password parameter is that I can't achieve the code coverage target, so the CodeCov checks will fail unfortunately. The untested code occurs after the call to the keyring which would fail as there is no keyring backend in the GitHub CI environment.

yeah, no worries about the coverage test, those are not run with remote-data anyway, so always show a much smaller percentage as the reality.

I'll open a follow-up issue to get a backend installed in the VM, it shouldn't be too hard (though populating it may not be trivial).

pllim · 2022-05-04T13:17:49Z

astroquery/casda/tests/test_casda.py

+    with pytest.raises(LoginError) as excinfo:
+        Casda.login()
+
+    assert "If you do not pass a username to login()," in str(excinfo.value)


Suggested change

with pytest.raises(LoginError) as excinfo:

Casda.login()

assert "If you do not pass a username to login()," in str(excinfo.value)

with pytest.raises(LoginError, match=r"If you do not pass a username to login\(\),") as excinfo:

Casda.login()

I've made this change for all exception tests

bsipocz · 2022-05-06T00:28:30Z

I'm sorry for merging the conflicting PR before this one, I missed noticing that it will cause issues. However, I'll go ahead and rebase this before merge to get a clear, linear history instead of the 3 merges.

bsipocz · 2022-05-06T00:40:36Z

(It looks like the email address you did these commits are not recognized by github, thus the weird avatar listing on them. In the git history, they all look good, with you as the author. I guess your avatar would also come up if you were to add that other email address to your GH account (you can have multiple ones associated)).

jd-au · 2022-05-06T00:46:08Z

(It looks like the email address you did these commits are not recognized by github, thus the weird avatar listing on them. It the git history it all look good, with you as author. I guess your avatar would also come up if you were to add that other email address to your GH account (you can have multiple ones associated)).

Thanks for letting me know. I've corrected it locally now but will also update my GH account.

bsipocz

The remaining follow-up item requires to get keyring work on CI, so I would not hold this up any longer.

bsipocz · 2022-05-06T01:05:43Z

When running the tests with remote-data=any, the coverage is indeed much higher, 88%.

I'll look into installing a keyring backend to CI, but that should not hold this one up.

bsipocz · 2022-05-06T01:05:51Z

Thank you @jd-au!

jd-au · 2022-05-06T01:09:41Z

Once the keyring backend is installed I'll be able to use the cached_credentials fixture from the remote tests to test the login code properly.

Thanks @bsipocz

jd-au added enhancement casda labels Apr 30, 2022

jd-au added this to the v0.4.7 milestone Apr 30, 2022

jd-au commented Apr 30, 2022

View reviewed changes

bsipocz reviewed May 2, 2022

View reviewed changes

pllim reviewed May 4, 2022

View reviewed changes

bsipocz mentioned this pull request May 4, 2022

MNT: Remove explicit password arguments from methods #2391

Open

jd-au added 5 commits May 5, 2022 17:32

Switch CASDA to QueryWithLogin

81197c3

Tidy up and add change log entry

8ea2111

Add default cutout radius

284a3d2

Remove password parameter

2c6ffb7

Update exception tests to use match

95b41d1

bsipocz force-pushed the CASDA-Use-QueryWithLogin branch from ed57149 to 95b41d1 Compare May 6, 2022 00:34

Adding jd-au to mailmap

740b05a

bsipocz approved these changes May 6, 2022

View reviewed changes

bsipocz merged commit ad6cb80 into astropy:main May 6, 2022

Uh oh!

Switch CASDA to QueryWithLogin #2386

Switch CASDA to QueryWithLogin #2386

Uh oh!

Conversation

jd-au commented Apr 30, 2022

Uh oh!

pep8speaks commented Apr 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2022-05-06 00:38:43 UTC

Uh oh!

codecov bot commented Apr 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsipocz May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jd-au May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsipocz commented May 6, 2022

Uh oh!

bsipocz commented May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jd-au commented May 6, 2022

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

bsipocz commented May 6, 2022

Uh oh!

bsipocz commented May 6, 2022

Uh oh!

jd-au commented May 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pep8speaks commented Apr 30, 2022 •

edited

Loading

codecov bot commented Apr 30, 2022 •

edited

Loading

bsipocz May 6, 2022 •

edited

Loading

jd-au May 4, 2022 •

edited

Loading

bsipocz commented May 6, 2022 •

edited

Loading