-
Notifications
You must be signed in to change notification settings - Fork 7
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 api endpoint for asset recompile #84
Conversation
@@ -25,7 +25,8 @@ | |||
subject | |||
extension.reload | |||
expect(extension.owner_name).to be_present | |||
expect(extension.owner_name).to_not eql orig_owner_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 do not want to remove this test, because setting the extension's owner name is one of the responsibilities of the SetUpHostedExtension
service class.
@@ -3,7 +3,7 @@ | |||
association :category | |||
association :owner, factory: :user | |||
sequence(:name) { |n| "redis-#{n}" } | |||
sequence(:owner_name) { |n| "owner#{n}" } |
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.
See my comment below about leaving the "sets the extension's owner name"
test in place. If this line in the extension factory is causing grief in some other test, we should look at why the grief is occurring. In other words, the application code cannot count on the owner name being the same as the owner.username
, so I don't want to embed that mistaken assumption into the test factory.
@@ -123,6 +123,7 @@ en: | |||
authentication_request_error: "Authentication failed due to an invalid signed request." | |||
authentication_key_error: "Authentication failed due to an invalid public/private key pair. If you have changed your keys recently try logging out and logging back in to Bonsai." | |||
unauthorized_upload_error: "You are not authorized to upload this asset." | |||
unauthorized_recompile_error: "You are not authorized to upload this asset." |
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.
Should the text read "You are not authorized to recompile this asset"?
get 'assets/:username/:id' => 'extensions#show', as: :extension | ||
get 'assets/recompile/:username/:id' => 'extensions#sync_repo', as: :extension_recompile |
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.
Let's make this a PUT request instead of a GET request, since it has a side effect and is idempotent.
) | ||
end | ||
|
||
if ENV['OAUTH_ACCOUNT_PROVIDER'] == 'chef_oauth2' |
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'm fuzzy on the new OAUTH_ACCOUNT_PROVIDER
environment variable and why we need to crowbar the authentication with it. Can we chat about this?
rescue | ||
render_not_authorized([t('api.error_messages.unauthorized_recompile_error')]) | ||
else | ||
SyncExtensionRepoWorker.perform_async(extension.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.
The SyncExtensionRepoWorker
only works for GitHub-based extensions. We need this new API endpoint to work for hosted extensions, too.
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.
To make this code work for hosted- and GitHub-based extensions, you can call the new CompileExtension
service class in #86.
Need to add OAUTH_ACCOUNT_PROVIDER=github to ENV on local and Heroku.