-
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
Running validate after a command finishes #1976
Conversation
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 great, just minor documentation comments.
def _complete_artifact_definition(self, artifact_data): | ||
""""Performs the needed steps to complete an artifact definition job | ||
|
||
In order to complete an artifact definition job we ned to create |
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.
ned -> need
for artifact in self.input_artifacts: | ||
templates.update(pt.id for pt in artifact.prep_templates) | ||
if len(templates) > 1: | ||
raise qdb.exceptions.QiitaDBError( |
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.
Can you add this exception in the Raises
section of the docstring?
👍 looks good to me. |
The error is real (already restarted the job once), @josenavas can you fix it? |
Yeah - I'm working on it - while testing on the test environment I found an issue and I'm still trying to figure out where the issue resides, but I think it is a bug in the qiita plugins. |
FOR cmd IN | ||
SELECT command_id FROM qiita.software_command WHERE name = 'Validate' | ||
LOOP | ||
INSERT INTO qiita.command_parameter (command_id, parameter_name, parameter_type, required, default_value) |
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 this add the actual provenance value to the already created artifacts?
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.
Note that this is adding a parameter called "provenance" to the validate command. The command itself doesn't use it but it is used internally in Qiita to differentiate a validate command that has been generated as a result of the user uploading data to Qiita from a validate command that has been generated as a result of an "artifact transformation" job finishing. This doesn't really contain the provenance of the artifact, so no need to add to the already created artifacts.
Note that the provenance of the artifacts is stored on the DAG structure in the DB.
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.
Got it, thanks. Just to confirm, this value is not required or defined by the artifacts, they are defined as part of the Validate command that generated those artifacts, 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.
Correct
Looks good, one question. |
This PR removes the assumption that plugins will generate correct artifacts, so after a command finishes, it will call the validate command of the given artifact type. This allows us to make sure that each artifact is fully correct, as well as enabling generating some stuff automatically (e.g. the HTML summary).
Please, review this PR but hold a minute before merging - I'm testing it on the test environment, but if this gets reviewed we get a head start!