-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add helpers #72
Add helpers #72
Conversation
Signed-off-by: Pablo Chacin <[email protected]>
Signed-off-by: Pablo Chacin <[email protected]>
Signed-off-by: Pablo Chacin <[email protected]>
Signed-off-by: Pablo Chacin <[email protected]>
Signed-off-by: Pablo Chacin <[email protected]>
Signed-off-by: Pablo Chacin <[email protected]>
Signed-off-by: Pablo Chacin <[email protected]>
Signed-off-by: Pablo Chacin <[email protected]>
Signed-off-by: Pablo Chacin <[email protected]>
Signed-off-by: Pablo Chacin <[email protected]>
Signed-off-by: Pablo Chacin <[email protected]>
Signed-off-by: Pablo Chacin <[email protected]>
Signed-off-by: Pablo Chacin <[email protected]>
Dynamic client's watch returns a channel of events that hold a generic object. This makes the function less useful as an structured function. Signed-off-by: Pablo Chacin <[email protected]>
Signed-off-by: Pablo Chacin <[email protected]>
Signed-off-by: Pablo Chacin <[email protected]>
Signed-off-by: Pablo Chacin <[email protected]>
Signed-off-by: Pablo Chacin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some initial, high-level comments before engaging in a full review 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the most part, I like the flow of the API, but there are a couple of items noted making for clunky usage.
@@ -67,6 +67,7 @@ This API offers methods for creating, retrieving, listing and deleting resources | |||
| | namespace | | |||
| list | kind| returns a collection of resources of a given kind | |||
| | namespace | | |||
| update | spec object | updates an existing resource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should apply
simply perform either a create or update depending upon existence? Have explicit calls to create
fail if a resource is already there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apply recieves a yaml. Update receives an object. These are two different operations.
README.md
Outdated
| Method | Parameters| Description | | ||
| -------------| ---| ------ | | ||
| helpers | | returns helpers that operate in the default namespace | | ||
| namespacedHelpers | namespace | returns helpers that operate in the given namespace | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having both the helpers
and namespacedHelpers
feels awkward. I'd prefer default
namespace be used unless explicitly provided to helpers()
.
README.md
Outdated
kubernetes.create(pod) | ||
|
||
// get helpers for test namespace | ||
const helpers = kubernetes.namespacesHelpers(ns) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be namespacedHelpers(ns)
? (note "namespaced" versus "namespaces".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap! thanks for catching this.
Signed-off-by: Pablo Chacin <[email protected]>
I have addressed your concerns regarding the use of namespaces in the helpers:
|
examples/wait_pod_running.js
Outdated
const kubernetes = new Kubernetes(); | ||
|
||
// create namespace with random name with prefix 'test-' | ||
const ns = kubernetes.helpers().createRandomNamespace("test-") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Script needs to be updated for latest changes. The createRandomNamespace
method is no longer valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Fixed
TODO: We need to try to find a way for the example scripts to be executed in the build pipeline as some sort of integration test. |
Totally agree. There is issue #48. I just added a comment on how e2e tests are implemented in xk6-disruptor. We could use that for running scripts as e2e tests. |
Signed-off-by: Pablo Chacin <[email protected]>
This change set introduces helpers to facilitate common tasks when setting tests in Kubernetes.
It also introduces a set of structured operations that work on typed objects (e.g. Pods) as opposed
to unstructured operations that work on generic objects. These structured operations are intended for
the helpers, to avoid the complexity of manipulating generic objects in go.