[ETL-312] Ignore specific non-severe Android validation errors #112
Conversation
philerooski
commented
Jan 3, 2023
•
philerooski
commented
- Ignores Android validation errors as outlined in ETL-312.
- Minor refactor of Github upload-and-deploy workflow.
| "'appName' is a required property" in metadata_errors and | ||
| "'files' is a required property" in metadata_errors): | ||
| return False | ||
| if "taskData.json" in validation_result["errors"]: |
There was a problem hiding this comment.
Nit. Use .get(....) then you don't need the extra if statement.
There was a problem hiding this comment.
Normally, yes. But I'm checking for inequality (if not errors == ...) so unless I made the non-severe error the default return value of the get method then in the case that there were no taskData.json errors, the inequality would evaluate to true and is_expected_validation_error would prematurely return False. The way it's written now is more readable and less redundant imo than duplicating the non-severe errors in the arguments to get. Although, I think the not == syntax is a bit unconventional for single comparisons, so I will change that.
|
@rxu17 can you take a look at this too? |
| if "motion.json" in validation_result["errors"]: | ||
| motion_errors = validation_result["errors"]["motion.json"] | ||
| if not motion_errors == [ | ||
| "'type' is a required property"]: | ||
| return False |
There was a problem hiding this comment.
Should motion.json be checking against the same error as taskData.json? According to BMC-432 it seems like each MotionRecord also has an extra property called type?
Also the ticket mentions stepPath as a required property as well for motion.json that is missing for some records, would that be considered a non-severe error we would want to let through?
There was a problem hiding this comment.
That's right. The complete list of errors is tracked here: https://sagebionetworks.jira.com/browse/BMC-432
I'll need to think about which of these qualifies as non-severe, but at least one of them which I've left out does.
