Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
#10610base: develop
Are you sure you want to change the base?
[ENG-5430] ENABLE_GV feature flipping for
get_addon
#10610Changes from 3 commits
d194c91
671f188
c60bd66
230be4d
4425024
bc3fd85
e888e87
9d1bbe5
a23cf78
41c0e85
cdaafec
9bd6ad5
82d54ae
a73a069
d358809
3b164ff
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 thedisplay_name
.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.
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
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)
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.
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 endpointsThere 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.