docs: grab workload version in k8s tutorial#2559
Conversation
|
While working on this task, I noticed that in Chapter 5, we introduced |
Oops, you're quite right. Well spotted! In the tutorial we say "Then import |
Thanks for the confirmation David. I added the step to run |
dwilding
left a comment
dwilding
left a comment
There was a problem hiding this comment.
This is a serious chunk of work - thanks for working through it so thoroughly! It's looking great. I'm sending a first batch of comments and will hold off reviewing further until you've had a chance to think about them.
james-garner-canonical
left a comment
james-garner-canonical
left a comment
There was a problem hiding this comment.
This is looking good. I agree with David's suggestions.
|
Thanks everyone for the reviews. I really appreciate it ! I will address the feedback |
…e up; use a hard code message for BlockedStatus; add explanation on why mock_get_version is use
dwilding
left a comment
dwilding
left a comment
There was a problem hiding this comment.
I'm holding off approving until the collect-status question is settled, but the latest changes look good 🙂
I haven't cross-checked the doc pages against the charm source. I'd be very happy to do that as a final review step.
dwilding
left a comment
dwilding
left a comment
There was a problem hiding this comment.
Very nice indeed! I cross-checked the details and found a couple of small adjustments needed.
james-garner-canonical
left a comment
james-garner-canonical
left a comment
There was a problem hiding this comment.
This is looking good, it's nice to add this feature to the K8s tutorial (and more complicated to do so than in the machine tutorial both due to the container architecture and the tutorial structure).
I have a few questions and suggestions. @dwilding could you please take a look at my suggestions too in case your opinion differs.
| return | ||
| try: | ||
| version = fastapi_demo.get_version(config.server_port) | ||
| except RuntimeError as version_e: | ||
| logger.error("Failed to get workload version: %s", version_e) | ||
| self.unit.status = ops.BlockedStatus( | ||
| "Failed to get version from server: check port config" | ||
| ) | ||
| return | ||
| self.unit.set_workload_version(version) | ||
| ``` | ||
|
|
||
| We removed three `self.unit.status = ` lines from this version of the method. We'll handle replacing those shortly. |
There was a problem hiding this comment.
The following text for this code block is now kind of contradictory -- it's explaining that we removed self.unit.status = lines (presumably switching to the collect status pattern), but we're conspicuously leaving a self.unit.state = ops.BlockedStatus without further explanation.
If this is the right pattern, then I think it could use some explanation as to why it's not replaced.
But is it the right pattern? It looks like our collect status handler will happily clobber this blocked status with active status.
There was a problem hiding this comment.
But is it the right pattern? It looks like our collect status handler will happily clobber this blocked status with active status.
Is that true? I had assumed that the collect-status handler wouldn't override higher priority status with a lower priority one - but I actually don't know.
In tutorial text we say:
We also want to clean up the code to remove the places where we're setting the status outside of this method, other than anywhere we're wanting a status to show up during the event execution (such as
MaintenanceStatus).
Which suggested to me that a maintenance status would survive collect-status. And so presumably blocked status would too.
I think it would be good to test this to confirm.
There was a problem hiding this comment.
Oh but I suppose the point is that maintenance status gets overridden after the work in the event execution has completed. Hmm
| self.unit.status = ops.ActiveStatus() | ||
| self.unit.set_workload_version(version) |
There was a problem hiding this comment.
Very minor, but perhaps self.unit.status = ops.ActiveStatus() should be the very last line?
There was a problem hiding this comment.
Yep, initially self.unit.status = ops.ActiveStatus() is the last line.
However, we decided that putting self.unit.set_workload_version(version) last would simplify the tutorial, and went with it.
In chapter 1, section Set the workload version, we can simply write
In `src/charm.py`, append the following lines to the `_on_demo_server_pebble_ready` function:
```python
# Set the workload version of this charm.
version = fastapi_demo.get_version(port=8000)
self.unit.set_workload_version(version)
```
And this code propagated into Chapter 2 as you see here, and I kept it like that.
…alhost in get_version
4 participants
Closes #2282.
This PR modify the
From zero to hero: Write your first Kubernetes charmtutorial:The PR reviewer can focus on the Markdown docs. We can leave the code changes in
examples/at the end, when we have finalized the tutorial, as the changes inexamples/are plenty and mostly reflect the tutorial.