Skip to content
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

ALMA: auth handling to login to Keycloak #2712

Merged
merged 6 commits into from
Jun 1, 2023
Merged

Conversation

at88mph
Copy link
Contributor

@at88mph at88mph commented Apr 24, 2023

Added auth handling to login to Keycloak.

Fixes #2670

@codecov
Copy link

codecov bot commented Apr 24, 2023

Codecov Report

Merging #2712 (69e5251) into main (e7c684d) will increase coverage by 0.17%.
The diff coverage is 83.92%.

@@            Coverage Diff             @@
##             main    #2712      +/-   ##
==========================================
+ Coverage   65.87%   66.05%   +0.17%     
==========================================
  Files         233      233              
  Lines       17904    17925      +21     
==========================================
+ Hits        11794    11840      +46     
+ Misses       6110     6085      -25     
Impacted Files Coverage Δ
astroquery/alma/core.py 55.57% <83.92%> (+6.31%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@keflavich
Copy link
Contributor

@andamian could you provide the review for this one? It looks good to me but I haven't tested and I know nothing about the ALMA login protocol.

@bsipocz bsipocz changed the title Fixes #2670 ALMA: update URLs Apr 25, 2023
@bsipocz bsipocz added the alma label Apr 25, 2023
@bsipocz bsipocz added this to the v0.4.7 milestone Apr 25, 2023
@bsipocz bsipocz changed the title ALMA: update URLs ALMA: auth handling to login to Keycloak Apr 25, 2023
astroquery/alma/core.py Outdated Show resolved Hide resolved
astroquery/alma/core.py Outdated Show resolved Hide resolved
astroquery/alma/core.py Outdated Show resolved Hide resolved
astroquery/alma/core.py Show resolved Hide resolved
password : str
The user's password
"""
data = {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could probably return here if self._auth_host is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might be true. If the self._auth_host is set, that just means that self.get_valid_host() was called at some point, not that the current user is logged in.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Please ignore my comment.

astroquery/alma/core.py Outdated Show resolved Hide resolved
@andamian
Copy link

andamian commented Jun 1, 2023

@bsipocz - FYI: this is ready from our perspective.

@bsipocz
Copy link
Member

bsipocz commented Jun 1, 2023

I did a rebase to remove the merge commit, and run the remote tests.

Thank you @at88mph and @andamian! I see two, genuine-looking alma failures, but both are unrelated and are present on main, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants