Code review comment for lp:~blake-rouse/maas/fix-1581130
Revision history for this message
| Blake Rouse (blake-rouse) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
35a32c5
(Get the code!)
> This looks fine, but there's no explanation why much of it is necessary:
>
> - Stripping "custom/" seems a fairly arbitrary thing to do. The clean_name
> method has a docstring and a comment which are both useful like "2 + 2 # add
> 2 and 2" is. There needs to be a proper explanation for future maintainers in
> the code.
Add a better docstring to explain why.
>
> - What was the issue with NodeActionError? One test has changed (for two
> distinct source changes...) and I can't tell how things are better now.
The issue is that "message" attribute no longer exists on ValidationError in Django 1.8. The test was setting it up incorrect.
>
> - How come boot resources don't have a title? Is that a bug in simplestreams?
This is for uploaded images that do not come from simplestreams. A title on an uploaded image is optional. It is only used when the user wants to give it a nice title for the user to select in the WebUI.
>
> +1, just explain in the code why these changes are necessary.