Skip to content
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

Reimplement apply #81

Merged
merged 3 commits into from
Dec 2, 2022
Merged

Reimplement apply #81

merged 3 commits into from
Dec 2, 2022

Conversation

pablochacin
Copy link
Contributor

@pablochacin pablochacin commented Dec 1, 2022

Closes #79 #77

Note to reviewers
Due to limitations in the fake client (see kubernetes/client-go#1184 and kubernetes/client-go#970), unit tests for the apply method fail. The way to test this change set is running the following script:

test script for creating and updating pod (click to display code) ```js import { Kubernetes } from 'k6/x/kubernetes';

const manifest = `
apiVersion: v1
kind: Pod
metadata:
name: busybox
namespace: default
spec:
containers:

  • name: busybox
    image: busybox:1.23
    command: ["sleep", "300"]
    `

const update = `
apiVersion: v1
kind: Pod
metadata:
name: busybox
namespace: default
spec:
containers:

  • name: busybox
    image: busybox:1.24
    command: ["sleep", "300"]
    `

export default function () {
const kubernetes = new Kubernetes();

kubernetes.apply(manifest)
const created = kubernetes.get("Pod", "busybox", "default");
console.log(created.spec.containers[0].image)

kubernetes.apply(update)
const updated = kubernetes.get("Pod", "busybox", "default");
console.log(updated.spec.containers[0].image)

if (created.spec.containers[0].image == updated.spec.containers[0].image) {
throw "image not updated"
}
}


</details>

@pablochacin pablochacin requested review from javaducky and removed request for javaducky December 1, 2022 13:20
@pablochacin pablochacin marked this pull request as draft December 1, 2022 13:22
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
@pablochacin pablochacin marked this pull request as ready for review December 1, 2022 13:40
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Copy link
Contributor

@javaducky javaducky left a comment

Choose a reason for hiding this comment

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

👍 Tested by creating a couple of different namespaces. One iteration using Create then Apply to perform changes. Performed another iteration by using Apply to create the namespace, then using Apply again to modify by adding labels.

@@ -184,28 +184,44 @@ spec:
}

func TestApply(t *testing.T) {
// Skip test. see comments on test cases why
t.Skip()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just remove this test case completely (or comment out with a TODO noted) rather than have the skip. Having this here and not noticing the skip gives a false sense of security.

Copy link
Contributor Author

@pablochacin pablochacin Dec 2, 2022

Choose a reason for hiding this comment

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

I would prefer not to remove it, as the test it is needed. And the test itself "works", but fails due to issues with the test mocks. But I don't have a strong opinion here (maybe I'm used to see tests skipped due to flakiness)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apply fails if the resources already exist
2 participants