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

Consolidate all K8s API calls in one place #71

Merged
merged 10 commits into from
May 28, 2019

Conversation

nolar
Copy link
Contributor

@nolar nolar commented May 24, 2019

Issue : #13

This is purely a refactoring: moving all Kubernetes-client-related routines into one place.

And a lot of new tests for these K8s-client call (in addition to the existing ones).

Previously, the API calls were spread all over the codebase. And the official Kubernetes client is quite wordy, so the places were also unnecessary wordy (not to their main point).


Few notes:

The peering objects (cluster-scoped, namespace-scoped, and the legacy one) use exactly the same unified logic now, with no exceptions.

There are no changes in the APIs, DSLs, signatures, behaviour or anything user-facing or k8s-facing. Only the internal code moves. Except for this:

  • When patching AND the object is a cluster-scoped resource (no namespace by design), patch_cluster_custom_object() is used instead of patch_namespaced_custom_object(namespace=None), which would fail. This would also affect the patching of peering objects (cluster-scoped and legacy), but is fixed in place.

Patching of a namespaced resource uses the namespace of the resource itself (even for the cluster-scoped operator).


This PR is also a part of the following chain of changes:

  • (This PR): Extraction of the K8s-API-related methods to a single place, both for served objects and for peering objects (the logic must be aligned).
    • (Needed for): Testing of peering logic, regardless of the API calls. Tests automation #13
    • (Needed for): Testing of how the framework handles the events and which API methods it calls (mocking the central place instead of individual API client calls all around). Tests automation #13
      • (Needed for): Safe changes of the handling logic (i.e. silent spies, etc). <<< MAIN GOAL.
    • (Needed for): Switching to pykube-ng (Consider pykube-ng? #15), and generally being K8s-client-agnostic.
    • (Needed for): Making the whole framework async, eliminating the blocking API calls (or putting them to the asyncio thread executors).

@nolar nolar added the automation CI/CD: testing, linting, releasing automatically label May 24, 2019
@zincr
Copy link

zincr bot commented May 24, 2019

🤖 zincr found 0 problems , 1 warning

ℹ️ Large Commits
✅ Approvals
✅ Specification
✅ Dependency Licensing

Details on how to resolve are provided below


Large Commits

Checks all commits for large additions to a single file. Large commits should be reviewed more carefully for potential copyright and licensing issues

This file contains a substantial change, please review to determine if the change comes from an external source and if there are any copyright or licensing issues to be aware of

  • ℹ️ kopf/k8s/fetching.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ tests/k8s/test_patching.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
     

@nolar nolar added this to the 1.0 milestone May 24, 2019
@nolar nolar force-pushed the extract-apis-no-scopes branch 4 times, most recently from 4182d4b to 64d9197 Compare May 24, 2019 14:21
@nolar nolar changed the title Refactor the resource scopes for handlers and API methods used Consolidate all K8s API calls in one place May 24, 2019
@nolar nolar marked this pull request as ready for review May 24, 2019 14:38
@nolar nolar requested a review from samurang87 as a code owner May 24, 2019 14:38
@nolar nolar mentioned this pull request May 25, 2019
19 tasks
type=type,
reason=reason,
note=message,
# message=message,

Choose a reason for hiding this comment

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

are you leaving these comments intentionally? :)

Copy link
Contributor Author

@nolar nolar May 28, 2019

Choose a reason for hiding this comment

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

Yes.

I struggle to identify which fields do what here, as they are duplicating each other (but not all of them): e.g. note ~ message, regarding ~ related, event_time ~ deprecated_first_timestamp; also some are of unclear purpose, and are not shown anywhere, e.g. action.

I will get back to them later, probably when I will be switching Kopf to pykube-ng — just to see how it is better done with other clients.

This might also be needed for multi-version K8s compatibility, as these fields look like some transitional stage between Kubernetes versions, so it might happen that these lines should be uncommented later. — E.g., most likely, it will be a switch from V1beta1Event to something like V1Event class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samurang87 And immediately related: #81 — with the cleanup of these fields (on another occasion).

@nolar nolar merged commit 0c55dcb into zalando-incubator:master May 28, 2019
@nolar nolar deleted the extract-apis-no-scopes branch May 28, 2019 14:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automation CI/CD: testing, linting, releasing automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants