-
Notifications
You must be signed in to change notification settings - Fork 80
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
Plugin installation endpoints #1958
Plugin installation endpoints #1958
Conversation
…into plugin-installation-endpoints
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.
Looks good, just one comment and you need to fix the flake8 errors.
plugin = qdb.software.Software.from_name_and_version(name, version) | ||
except qdb.exceptions.QiitaDBUnknownIDError: | ||
raise HTTPError(404) | ||
except Exception as e: |
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.
It seems like all 500 errors are missing tests. Could you double check?
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 unsure on how to force those errors to happen. I put those as a security guard in case that something unexpected happens (that's why they catch any exception). Other endpoints are similar to this.
Any idea on how to force the error?
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.
If it's not possible to generate them, why test them?
Now, this is working on from_name_and_version
, which only raises QiitaDBUnknownIDError
, so the second except is working on:
sql = """SELECT software_id
FROM qiita.software
WHERE name = %s AND version = %s"""
qdb.sql_connection.TRN.add(sql, [name, version])
res = qdb.sql_connection.TRN.execute_fetchindex()
which will only fail if the DB fails or the structure changes, right? If that's correct I think we can remove them, right?
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.
If the DB fails, what will be then returned to the client?
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.
If the DB fails, the Qiita server will fail, right? So no return at all ...
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.
Not necessarily - our fault tolerant system allows the webserver to still be alive even when the DB is dead.
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.
K
Changes Unknown when pulling de87d41 on josenavas:plugin-installation-endpoints into * on biocore:plugin-installation*. |
This adds the needed endpoints so plugins can register themselves using the REST api.
@antgonza there are 2 missing things in here:
parameters
using the correct formatAdd a new function in the class qiita_db.software.Command calledActually not needed, there is the DefaultParameters.create calladd_default_parameter_sets
that adds new parameter sets to the command. Then you can call this function in the previous handler once the command is created.