Skip to content
This repository has been archived by the owner on Sep 14, 2020. It is now read-only.

Update the EphemeralVolumeClaim story-telling #97

Merged
merged 8 commits into from
Jan 30, 2020

Conversation

nolar
Copy link
Contributor

@nolar nolar commented Jun 2, 2019

This is a work in progress. Preview, but do not review.

Issue : #12

Description

The docs were showing some basic Kopf features, but they were not actually tested to work, both syntactically, and functionally.

This PR fixes all the issues with the EphemeralVolumeClaim in the docs tutorial (step by step example of developing and operator).

The actual operator is now implemented and manually tested here: https://github.com/nolar/ephemeral-volume-claims — all code snippets are taken from this repo.

A few sentences describing the overall goals of the pull request's
commits.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • Documentation / non-code

Tasks

@nolar nolar added the documentation Documentation improvements label Jun 2, 2019
@nolar nolar added this to the 1.0 milestone Jun 2, 2019
@zincr
Copy link

zincr bot commented Jun 2, 2019

🤖 zincr found 0 problems , 0 warnings

✅ Large Commits
✅ Approvals
✅ Specification
✅ Dependency Licensing

@zincr
Copy link

zincr bot commented Jun 2, 2019

🤖 zincr found 1 problem , 0 warnings

❌ Approvals
✅ Large Commits
✅ Specification
✅ Dependency Licensing

Details on how to resolve are provided below


Approvals

All proposed changes must be reviewed by project maintainers before they can be merged

Not enough people have approved this pull request - please ensure that 1 additional user, who have not contributed to this pull request approve the changes.

  • ✅ Approved by PR author @nolar
  • ❌ 1 additional approval needed
     

Copy link

@yashbhutwala yashbhutwala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to struggle through the docs, and I was about to submit a PR, then I saw this :-). It seems this is good enough to update the live docs, maybe merge it soon? So, someone else doesn't suffer as me haha


.. warning::
Kubernetes & ``kubectl`` improperly show the capacity of PVCs:
it remains the same (1G) event after the change.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

event --> even


@kopf.on.create('zalando.org', 'v1', 'ephemeralvolumeclaims')
def create_fn(spec, meta, namespace, logger, **kwargs):
def create_fn(body, spec, meta, namespace, logger, **kwargs):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need body here. delete kopf.adopt(data, owner=body) because you only show it later in cascaded deletion.


name = meta.get('name')
size = spec.get('size')
if not size:
raise kopf.HandlerFatalError(f"Size must be set. Got {size!r}.")

path = os.path.join(os.path.dirname(__file__), 'pvc-tpl.yaml')
path = os.path.join(os.path.dirname(__file__), 'pvc.yaml')
tmpl = open(path, 'rt').read()
text = tmpl.format(size=size, name=name)
data = yaml.load(text)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yaml.load(text) --> yaml.load(text, Loader=yaml.FullLoader) because https://github.com/yaml/pyyaml/wiki/PyYAML-yaml.load(input)-Deprecation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, I saw that warning already few times. Will fix that too.

@nolar
Copy link
Contributor Author

nolar commented Jun 19, 2019

@yashbhutwala Sorry for the yet incomplete docs. This was not intended to make anyone struggling :-)

This PR assumes that #110 is finished — which is "almost there", close to the end (also needs #109 before).

Originally, I intended to also finish the actual ephemeral-volume-claim-operator with all the features.

But you are right, the docs consistency is important — especially since Kopf got some traction suddenly. I will merge this PR as soon as #109+#110 are ready, and will continue adding the missing docs separately.

@yashbhutwala
Copy link

No worries, @nolar. Let me know if you need help 😄

Copy link

@piratos piratos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

found this issue while running through the readthedocs walk-through


name = meta.get('name')
size = spec.get('size')
if not size:
raise kopf.HandlerFatalError(f"Size must be set. Got {size!r}.")

path = os.path.join(os.path.dirname(__file__), 'pvc-tpl.yaml')
path = os.path.join(os.path.dirname(__file__), 'pvc.yaml')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os is not previously imported

Copy link

@asmacdo asmacdo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it through the tutorial pretty smoothly with this PR, thank you!

@@ -53,6 +53,7 @@ We will use the official Kubernetes client library:
:linenos:
:caption: ephemeral.py

import os
import kopf
import kubernetes
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: IMO it might be worth a note pip install kubernetes.

@@ -74,11 +74,11 @@ and patches the PVC with the new size from the EVC::
@kopf.on.update('zalando.org', 'v1', 'ephemeralvolumeclaims')
def update_fn(spec, status, namespace, logger, **kwargs):

size = spec.get('create_fn', {}).get('size', None)
size = status.get('size', None)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This didn't work for me. I think it should be spec.get('size', None)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Solved.

@nolar nolar marked this pull request as ready for review January 28, 2020 11:37
@nolar
Copy link
Contributor Author

nolar commented Jan 28, 2020

This PR is waiting for so long, that I have lost track of what was left. So, let's merge it as is. The tutorial story has to be redesigned/reviewed/rewritten anyway — especially with the per-pod ephemeral inline volumes coming in Kubernetes 1.16 by default.

@nolar nolar changed the title Update the EphemeralVolumeClaim story-telling in the docs/tutorial Update the EphemeralVolumeClaim story-telling Jan 28, 2020
@nolar nolar requested a review from aweller January 30, 2020 15:05
@nolar nolar merged commit 6eb5a5a into zalando-incubator:master Jan 30, 2020
@nolar nolar deleted the 12-evc-story branch January 30, 2020 18:09
@nolar
Copy link
Contributor Author

nolar commented Feb 13, 2020

fixes #238

@nolar nolar mentioned this pull request Feb 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Documentation improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants