new: allow providing the image over attachment for IoT#1026
new: allow providing the image over attachment for IoT#1026
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1026 +/- ##
==========================================
+ Coverage 77.21% 77.33% +0.12%
==========================================
Files 116 116
Lines 12065 12122 +57
Branches 996 998 +2
==========================================
+ Hits 9316 9375 +59
+ Misses 2520 2519 -1
+ Partials 229 228 -1
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
ajzobro
left a comment
ajzobro
left a comment
There was a problem hiding this comment.
I know it's in draft, but it caught my eye :-)
| if not attachments_dir.is_dir(): | ||
| return None | ||
| files = sorted(p for p in attachments_dir.iterdir() if p.is_file()) | ||
| return files[0] if files else None |
There was a problem hiding this comment.
I don't think that taking the first file alphabetically is a very robust solution. If we intend to keep it this way, can you include commentary as to why this choice was made?
| has_url = "url" in data | ||
| has_urls = "urls" in data | ||
| if has_url and has_urls: | ||
| def validate_image_source(self, data, **_): |
There was a problem hiding this comment.
if this is a "boot" image, could we consider adding the classifier onto the variable name so as not to confuse it with other "image" contexts.
| has_urls = "urls" in data | ||
| if has_url and has_urls: | ||
| def validate_image_source(self, data, **_): | ||
| """Validate that at most one URL source is provided. |
There was a problem hiding this comment.
| """Validate that at most one URL source is provided. | |
| """Validate that at most one boot image source is provided. |
| if ( | ||
| "url" not in data | ||
| and "urls" not in data | ||
| and not data.get("attachments") |
There was a problem hiding this comment.
we seem to be missing an error message in the case where url(s) and an attachment have been provided. It seems reasonable (based on the comments above) that if the attachment will be used to the exclusion of the urls that if both a boot image provisioning attachment and a url/urls are provided that we should respond with an appropriate error rather than silently discard or otherwise not use the url/urls value(s).
| ) | ||
|
|
||
|
|
||
| class ZapperIoTCustomProvisionData(BaseZapperProvisionData): |
There was a problem hiding this comment.
Since both of these leverage the Base -- is it really the base that should be getting these new rules? Or should we have a ZapperIoT*ProvisionData class?
class ZapperIoTCustomProvisionData(BaseZapperProvisionData):
class ZapperIoTPresetProvisionData(BaseZapperProvisionData):
I have the same comments for both classes.
| def test_add_job_zapper_iot_custom_provision_data_attachment(mongo_app): | ||
| """Test that a job with zapper_iot_custom using an attachment works.""" | ||
| provision_data = { | ||
| "attachments": [{"agent": "provision/image.img"}], |
There was a problem hiding this comment.
see comments above about picking the first file alphabetically -- can we at least only look at *.img files if that's the standard?
| output = app.post("/v1/job", json=job_data) | ||
| assert HTTPStatus.OK == output.status_code | ||
|
|
||
|
|
There was a problem hiding this comment.
Add more tests or combinations of mutually exclusive things (url/urls/attachments)
2 participants
Description
The server side now allows the user to provide the boot image as at attachment, useful when there's no accessible URL hosting the artifact.
Resolved issues
Part of https://warthogs.atlassian.net/browse/ZAP-1436
Documentation
On server side
Web service API changes
N/A
Tests
Updated.