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

[ENG-5699] Framework for getting Addon Info from GV #10641

Merged
merged 8 commits into from
Jun 21, 2024

Conversation

jwalz
Copy link
Collaborator

@jwalz jwalz commented Jun 18, 2024

Purpose

Build a generic replacement for NodeSettings objects that can be populated from GravyValet

Changes

  • Added osf/external/gravy_valet/request_helpers.py which handles making requests to GV (including generating HMAC-signed auth headers) and parsing the results into a useable format -- notably including linking included resources
  • Added osf/external/gravyvalet/compat.py which defines dataclasses to serve as functional fakes for NodeSettings and addon App Configs with data populated from GravyValet instead of the osf.io database
  • Updated gv_mocks to support testing includes and to match the updated GV API spec where we've removed trailing slashes
  • Updated tests to make use of endpoint definitions in requests_helpers
  • Global use of "Fake" over "Mock" to avoid mental collision with expected Python Mock behavior

QA Notes

Please make verification statements inspired by your code and what your code touches.

  • Verify
  • Verify

What are the areas of risk?

Any concerns/considerations/questions that development raised?

Documentation

Side Effects

Ticket

@jwalz jwalz requested a review from aaxelb June 18, 2024 19:40
osf/external/gravy_valet/request_helpers.py Show resolved Hide resolved
osf/external/gravy_valet/request_helpers.py Outdated Show resolved Hide resolved
osf/external/gravy_valet/request_helpers.py Outdated Show resolved Hide resolved
osf/external/gravy_valet/compat.py Outdated Show resolved Hide resolved
osf/external/gravy_valet/compat.py Outdated Show resolved Hide resolved
osf/external/gravy_valet/gv_fakes.py Outdated Show resolved Hide resolved
osf/external/gravy_valet/request_helpers.py Outdated Show resolved Hide resolved
osf/external/gravy_valet/gv_fakes.py Outdated Show resolved Hide resolved
osf/external/gravy_valet/compat.py Outdated Show resolved Hide resolved
osf/external/gravy_valet/auth_helpers.py Outdated Show resolved Hide resolved
) -> str:
"""Returns the expected (status, headers, json) tuple expected by callbacks for MockRequest."""
"""Returns the expected (status, headers, json) tuple expected by callbacks for FakeRequest."""
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a lie!!!! Fix plz

@jwalz jwalz marked this pull request as ready for review June 20, 2024 18:49
Copy link
Contributor

@aaxelb aaxelb left a comment

Choose a reason for hiding this comment

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

looking good, only small things

Copy link
Contributor

Choose a reason for hiding this comment

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

regardless of name, this is only meant for use in tests (unless i'm missing something) -- mind moving it somewhere like osf_tests?

)
response = requests.get(endpoint_url, headers=auth_headers, params=params)
if not response.ok:
# log error to Sentry
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO?

osf/external/gravy_valet/translations.py Outdated Show resolved Hide resolved
Comment on lines 92 to 95
def get_gv_result(**kwargs): # -> JSONAPIResultEntry
'''Processes the result of a request to a GravyValet detail endpoint into a single JSONAPIResultEntry.

kwargs must match _make_gv_request
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm not crazy about obfuscating a call signature like this, but it's not not pythonic...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just got sick of typing the same signature over and over again -- will spell it out

Copy link
Contributor

@aaxelb aaxelb left a comment

Choose a reason for hiding this comment

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

🖼️

@jwalz jwalz merged commit 19845f8 into CenterForOpenScience:develop Jun 21, 2024
6 checks passed
Johnetordoff pushed a commit to Johnetordoff/osf.io that referenced this pull request Jul 25, 2024
 into remove-quickfiles-code

* 'develop' of https://github.com/CenterForOpenScience/osf.io: (166 commits)
  update dataverse dep revision to get changes
  Update CHANGELOG, bump version
  [CR][ENG-5681] Great Big Python Upgrade (CenterForOpenScience#10648)
  Revert "[ENG-3685] Add permissions for withdrawn registration files (CenterForOpenScience#10650)" (CenterForOpenScience#10666)
  Check Registration READ perms on the Registration - Do not record download metrics for renders
  Fix signature
  Allow DOI metadata updates to be queued
  [ENG-3685] Add permissions for withdrawn registration files (CenterForOpenScience#10650)
  Update CHANGELOG, bump version
  [ENG-5030] Preprints Phase 2 - BE (CenterForOpenScience#10617)
  Update CHANGELOG, bump version
  Ensure Assumed-HAM users do not get autobanned
  [ENG-5762] Get GV set up in osf docker configs (CenterForOpenScience#10643)
  [ENG-5718] Use `make_auth` to avoid assumptions about `auth.user` (CenterForOpenScience#10647)
  [ENG-5699] Framework for getting Addon Info from GV (CenterForOpenScience#10641)
  [ENG-5178] Allow unauthenticated users to see public files (CenterForOpenScience#10645)
  Fix RelationshipPostMakesNoChanges exception in project creation (CenterForOpenScience#10644)
  [ENG - 5008] Support Unicode and special characters in file names during archiving (CenterForOpenScience#10627)
  Set Default Resource Type for Registrations to "Study Registration" (CenterForOpenScience#10636)
  Configurable GV Mock + HMAC Auth (CenterForOpenScience#10623)
  ...

# Conflicts:
#	addons/base/views.py
#	api/caching/tasks.py
#	api_tests/files/views/test_file_detail.py
#	api_tests/wb/views/test_wb_hooks.py
#	osf/management/commands/data_storage_usage.py
#	osf/management/commands/reindex_quickfiles.py
#	osf/management/commands/transfer_quickfiles_to_projects.py
#	osf/models/__init__.py
#	osf/models/private_link.py
#	osf/models/quickfiles.py
#	osf/models/user.py
#	scripts/fix_merged_user_quickfiles.py
#	tests/test_views.py
#	website/mails/mails.py
#	website/search/elastic_search.py
#	website/settings/defaults.py
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.

None yet

2 participants