Enhance list blueprints #69
Conversation
There was a problem hiding this comment.
Looks good! You might not have time to address these so I think this should be okay to merge but I want to get @amauboussin 's eyes on it too
|
|
||
| create_params = {'template_id': self.id, 'name': name} | ||
| project = Project.create(**create_params) | ||
| blueprint = Blueprint._from_project(project) |
There was a problem hiding this comment.
Hmm - if you wanted to split the concept of a Blueprint project from a blueprint-derived project, could we split off the methods below this one, plus required_data_fields, into a separate class?
There was a problem hiding this comment.
It's a good question! I lingered over this for a while. But the real sticking point for me is the fact that list_blueprints was already returning Project objects. I was trying to paint "within the lines" as I saw them, and to my eye we already decided that Projects types were going to represent both Blueprints (or templates) and the Projects derived from them. If we decided that list_blueprints should return a non-Project Blueprint or Template object that would make sense to me, but it would be backward-incompatible which I assumed we didn't want in this PR.
| from surge.errors import SurgeMissingAttributeError | ||
|
|
||
|
|
||
| class Blueprint(Project): |
There was a problem hiding this comment.
Might be good to validate that launch/pause/cancel etc. in Project aren't called for a blueprint, either in this class or in Project (perhaps in a follow-up PR) https://github.com/CivJ/surge-python/blob/3134a80007faad6b524a28bdc2cf9885df0a1136/surge/projects.py#L200
There was a problem hiding this comment.
This is of course validated on the server side though so maybe it doesn't matter
There was a problem hiding this comment.
Hi Tim! Yea I was a little worried about this. But since a Blueprint project can either be "just the template" or a real project that was derived from a template, it seems like we'd want the full project API available in the second case. For the first case, I am hoping the server will protect us (which may not be a good assumption).
| b.created_at = created_at | ||
| return b | ||
|
|
||
| def required_data_fields(self): |
There was a problem hiding this comment.
This probably is outside the scope here but is this validated on the server side? I think we have fairly elaborate templating logic that could continue to evolve so this probably won't be exhaustive
Add Blueprint class to surge SDK
New Blueprint Class
To provide a simpler workflow for creating projects from Blueprints, we introduce Blueprints as a first class object. It contains three public methods:
Changes to Project Class
Comments
Blueprints represent both a template AND a project which has been created from the template. This might be a little confusing to users. However, since list_blueprints was already returning a Project object it seemed like we already decided it's ok to mix these two concepts.
The timeboxed nature of the exercise means less test coverage than I would have normally added.
Assumptions