-
Notifications
You must be signed in to change notification settings - Fork 327
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-5430] ENABLE_GV feature flipping for get_addon
#10610
base: develop
Are you sure you want to change the base?
[ENG-5430] ENABLE_GV feature flipping for get_addon
#10610
Conversation
0b9d7f6
to
934ea23
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next steps:
-
AddonModelMixin._settings_model
won't currently work. We actually need to make the request beforehand using thename
as the ID for theAuthorizedStorageAccount
(user.get_addon) or theConfiguredStorageAddon
(node.get_addon). If those serializers don't currently return the name of the connected service, they should. That can then be used as a lookup to get the correct SettingsModel. -
MockUserSettings and MockNodeSettings should populate based on data from the AuthorizedStorageAccount and ConfiguredStorageAddon (UserReference and ResourceReference just exist to link things and contain no configuration data).
-
(future ticket)
get_addons
should request all of the authorized storage accounts for a user or connected_storage_addons for a node instead of iterating through a list of provider names (and then pack a list whatever the appropriate serializer is based on the returned values).
website/settings/defaults.py
Outdated
GV_RESOURCE_DOMAIN = 'http://192.168.168.167:8004/v1/resource-references/?filter[resource_uri]={owner_uri}' | ||
GV_USER_DOMAIN = 'http://192.168.168.167:8004/v1/user-references/?filter[user_uri]={owner_uri}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GV_RESOURCE_DOMAIN = 'http://192.168.168.167:8004/v1/resource-references/?filter[resource_uri]={owner_uri}' | |
GV_USER_DOMAIN = 'http://192.168.168.167:8004/v1/user-references/?filter[user_uri]={owner_uri}' | |
GV_API_ROOT = 'http://192.168.168.167:8004/v1 | |
GV_RESOURCE_ENDPOINT = GV_DOMAIN + 'resource-references/?filter[resource_uri]={resource_uri}' | |
GV_USER_ENDPOINT = GV_DOMAIN + 'user-references/?filter[user_uri]={owner_uri}' | |
# These two are for `get_addon` vs `get_addons` | |
GV_USER_ADDON_ENDPOINT = GV_DOMAIN + 'v1/authorized-storage-accounts/{account_id}' | |
GV_NODE_ADDON_ENDPOINT = GV_DOMAIN + 'v1/configured-storage-addons/{addon_id}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could optionally append &include=authorized_storage_accounts|configured_storage_addons
to the user/resource endpoints
293e89a
to
221248b
Compare
221248b
to
d194c91
Compare
into gv-files-provider-waffle * 'develop' of https://github.com/CenterForOpenScience/osf.io: [ENG-5140] #2 Update get_auth for GV and readability (CenterForOpenScience#10613)
375673b
to
074878f
Compare
074878f
to
c60bd66
Compare
attributes = res.json['data']['attributes'] | ||
assert attributes['provider'] == str(provider_gv_id) | ||
assert attributes['name'] == str(provider_gv_id) | ||
assert res.json['data']['id'] == f'{file.target._id}:{str(provider_gv_id)}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want this to just be the provider_gv_id
. @brianjgeiger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for everything but (I believe) osfstorage, if there's a gv provider id, then that should be the whole id. Yay for unique provider ids.
assert attributes['provider'] == str(provider_gv_id) | ||
assert attributes['name'] == str(provider_gv_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brianjgeiger would we like either of these (probably name?) to be the display_name
of the addon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm using primarily gv information for this on the front end, but yeaaaahhh, let's try name
to be the display_name
.
@@ -2430,6 +2433,29 @@ def _remove_from_associated_collections(self, auth=None, force=False): | |||
force=True | |||
) | |||
|
|||
def get_addon(self, name, is_deleted=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My instinct is that, instead of having the subclasses completely override get_addon, the superclass should start with a check for the GravyValet flag and then invoke a _get_addon_from_gravyvalet or similar method that is defined on the subclass. That gets rid of the need to call super() and better delegates responsibility to the subclass
osf/models/node.py
Outdated
def get_addon(self, name, is_deleted=False): | ||
request, user_id = get_request_and_user_id() | ||
|
||
if hasattr(request, 'user') and not isinstance(request.user, AnonymousUser) and waffle.flag_is_active(request, features.ENABLE_GV) and name not in ['osfstorage', 'wiki']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to guard this against the AnonymousUser. Unauthenticated users should still be able to see the addons for a public node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is still true (this is also a delta between node and user behavior)
c7606c9
to
dc4cda4
Compare
into gv-files-provider-waffle * 'develop' of https://github.com/CenterForOpenScience/osf.io: Fix get auth number two (CenterForOpenScience#10614)
dc4cda4
to
bc3fd85
Compare
ff1e20b
to
65ebf16
Compare
65ebf16
to
e888e87
Compare
a2a6176
to
9d1bbe5
Compare
website/settings/defaults.py
Outdated
GV_USER_ADDON_ENDPOINT = 'http://192.168.168.167:8004/v1/authorized-storage-accounts/{account_id}' | ||
GV_NODE_ADDON_ENDPOINT = 'http://192.168.168.167:8004/v1/configured-storage-addons/{config_id}/base_account/' | ||
GV_EXTERNAL_STORAGE_ENDPOINT = 'http://192.168.168.167:8004/v1/external-storage-services/{service_id}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GV_USER_ADDON_ENDPOINT = 'http://192.168.168.167:8004/v1/authorized-storage-accounts/{account_id}' | |
GV_NODE_ADDON_ENDPOINT = 'http://192.168.168.167:8004/v1/configured-storage-addons/{config_id}/base_account/' | |
GV_EXTERNAL_STORAGE_ENDPOINT = 'http://192.168.168.167:8004/v1/external-storage-services/{service_id}' | |
GV_USER_ADDON_ENDPOINT = GV_API_ROOT + 'authorized-storage-accounts/{account_id}' | |
GV_NODE_ADDON_ENDPOINT = GV_API_ROOT + 'configured-storage-addons/{config_id}/base_account/' | |
GV_EXTERNAL_STORAGE_ENDPOINT = GV_API_ROOT + 'external-storage-services/{service_id}' |
website/settings/defaults.py
Outdated
GV_RESOURCE_DOMAIN = 'http://192.168.168.167:8004/v1/resource-references/?filter[resource_uri]={owner_uri}' | ||
GV_USER_DOMAIN = 'http://192.168.168.167:8004/v1/user-references/?filter[user_uri]={owner_uri}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GV_RESOURCE_DOMAIN = 'http://192.168.168.167:8004/v1/resource-references/?filter[resource_uri]={owner_uri}' | |
GV_USER_DOMAIN = 'http://192.168.168.167:8004/v1/user-references/?filter[user_uri]={owner_uri}' |
osf/models/node.py
Outdated
def get_addon(self, name, is_deleted=False): | ||
request, user_id = get_request_and_user_id() | ||
|
||
if hasattr(request, 'user') and not isinstance(request.user, AnonymousUser) and waffle.flag_is_active(request, features.ENABLE_GV) and name not in ['osfstorage', 'wiki']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is still true (this is also a delta between node and user behavior)
addons/base/utils.py
Outdated
class MockNodeSetting: | ||
def __init__(self, resource, auth, legacy_config, gv_data): | ||
self.gv_data = gv_data | ||
|
||
@property | ||
def configured(self): | ||
return True | ||
|
||
@property | ||
def config_id(self): | ||
return self.gv_data.config_id | ||
|
||
class MockUserSetting: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the benefit of making these internal classes? They don't seem to be using the enclosing scope in any way. Also, the majority of the init args are going unused.
addons/base/utils.py
Outdated
|
||
class GravyValetAddonAppConfig: | ||
class MockNodeSetting: | ||
def __init__(self, resource, auth, legacy_config, gv_data): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like:
def __init__(self, configured_storage_addon_id):
self.gv_data = requests.get(configured-storage-addons/configured-storage-addon-id/?include=external_storage_service)
self.legacy_app_config = _get_legacy_config_for_gv_service(gv_data.name)
self.magically_mirror_legacy_node_settings(legacy_app_config.node_settings_module)
@property
def config:
return self.legacy_app_config
"magically_mirror_legacy_node_settings" could come from a per-provider translator class/module that is surfaced with a standardized name on the legacy_config
7a55c02
to
9f0d6e4
Compare
647f35c
to
06cf8fe
Compare
06cf8fe
to
9bd6ad5
Compare
into gv-files-provider-waffle * 'develop' of https://github.com/CenterForOpenScience/osf.io: add DraftRegistration file permission test (CenterForOpenScience#10626) add DraftRegistration file permission test (CenterForOpenScience#10622)
into gv-files-provider-waffle * 'develop' of https://github.com/CenterForOpenScience/osf.io: Configurable GV Mock + HMAC Auth (CenterForOpenScience#10623)
Purpose
Allows us to feature flip between our current system and the new GravyValet addon service. Should change
files
related routes to use new set up.Changes
get_addon
functions for AbstractNode and OsfUser waffled to return mock configGravyValetAddonAppConfig
to serve as addon proxy objectQA Notes
Please make verification statements inspired by your code and what your code touches.
What are the areas of risk?
Any concerns/considerations/questions that development raised?
Documentation
Side Effects
Ticket
https://openscience.atlassian.net/browse/ENG-5430