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

add get_email call to __init__ #524

Merged
merged 2 commits into from
Nov 14, 2024
Merged

Conversation

OwenPriceSkelly
Copy link
Member

@OwenPriceSkelly OwenPriceSkelly commented Aug 20, 2024

fixes #523

Overview

This just adds a single get_email call to the GardenClient.__init__ method, which ensures that the client has authed with our backend at least once before attempting to pull a garden or entrypoint to run on the demo endpoint.

This makes it so you can't accidentally try running an entrypoint before the backend has had the chance to add you to the Garden Users globus group.

Discussion

This is definitely a bit of a hack, but so is the backend's automatically adding users to the group. If there's an obvious cleaner way to do this I'm open to it, this was just the first easy fix to come to mind.

Testing

I had to tweak the garden_client fixture so the call to BackendClient.get_user_info would be mocked with test user data. This fixed almost all of the tests except for one backend client test, since the backend_client fixture depends on the garden_client fixture (which now mocks the backend client method being tested).

I'll defer to the judgement of the test czar @hholb, but I decided it was ok to delete the single remaining affected test. It seemed like the logic it was testing of "an exception is raised by the backend" is something that could be (and already is, afaik) covered by the backend's unit tests, but I'm open to pushback on this!

Another more robust / time consuming option would be to make the backend_client fixture independent of the garden_client fixture, I just didn't think it was worth the effort for this PR


📚 Documentation preview 📚: https://garden-ai--524.org.readthedocs.build/en/524/

@OwenPriceSkelly OwenPriceSkelly force-pushed the owen/fix-auto-join-group-skip branch from 65ec55a to fe46f2c Compare August 23, 2024 19:33
Copy link

codecov bot commented Aug 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.91%. Comparing base (0244c64) to head (8482b41).
Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #524      +/-   ##
==========================================
- Coverage   74.94%   74.91%   -0.04%     
==========================================
  Files          38       38              
  Lines        2407     2408       +1     
==========================================
  Hits         1804     1804              
- Misses        603      604       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@OwenPriceSkelly OwenPriceSkelly force-pushed the owen/fix-auto-join-group-skip branch from fe46f2c to d1c494b Compare August 26, 2024 19:04
Copy link
Collaborator

@WillEngler WillEngler left a comment

Choose a reason for hiding this comment

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

Cool, let's do it!

@OwenPriceSkelly OwenPriceSkelly force-pushed the owen/fix-auto-join-group-skip branch from d1c494b to 8482b41 Compare November 14, 2024 21:22
@OwenPriceSkelly OwenPriceSkelly merged commit 3b7e1bc into main Nov 14, 2024
5 checks passed
@OwenPriceSkelly OwenPriceSkelly deleted the owen/fix-auto-join-group-skip branch November 14, 2024 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: it's possible to skip joining the "Garden Users" group
2 participants