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.

Sign up for GitHub

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

Jump to bottom

Add Kubectl delete tests #2906

Merged
merged 11 commits into from
Nov 28, 2023
Merged

Add Kubectl delete tests #2906

merged 11 commits into from
Nov 28, 2023

Conversation

awesominat
Copy link
Contributor

@awesominat awesominat commented Nov 23, 2023 •
edited
Loading

this PR aims to add tests to Kubectl.delete and its ignoreNotFound method.

I'm currently having issues getting it to work. I can't get Kubect.delete to recognize V1Job as a namespaced class if I set KubectlDelete\<V1Job>'s ApiClient. If I don't set the ApiClient, then executing delete fails to connect to local host.
I'm considering executing ModelMapper.addModelMap to add V1Job as namespaced class for Kubectl.delete to execute, but I'd like feedback on whether the changes I've made make sense or should be refactored.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 23, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 23, 2023
@awesominat
Copy link
Contributor Author

/cc @brendandburns

Hi, whenever you're free, can you give me your feedback on my changes?

@awesominat
Copy link
Contributor Author

I have hard coded more http returns, but now it passes even when it should error for the second call. I am able to get it to error if I create a normal file that isn't a test and use Kubectl there. I feel like I am missing a crucial part of mocking these http calls

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 25, 2023
@awesominat awesominat marked this pull request as ready for review November 25, 2023 00:59
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 25, 2023
brendandburns
brendandburns reviewed Nov 27, 2023
View reviewed changes
extended/src/test/java/io/kubernetes/client/extended/kubectl/KubectlDeleteTest.java Outdated
new File(
KubectlDeleteTest.class
.getClassLoader()
.getResource("discovery-api-v1.json")
Copy link
Contributor

Choose a reason for hiding this comment

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

can you use getResourceAsStream() and load this into a byte array instead of the File stuff, I find it confusing to read.

brendandburns
brendandburns reviewed Nov 27, 2023
View reviewed changes
extended/src/test/java/io/kubernetes/client/extended/kubectl/KubectlDeleteTest.java Outdated
.willReturn(
aResponse()
.withStatus(201)
.withBody(new String(Files.readAllBytes(Paths.get(ADD_JOB))))));
Copy link
Contributor

Choose a reason for hiding this comment

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

Once you convert to byte array/string above, you can simplify this code.

@brendandburns
Copy link
Contributor

Thanks for adding tests!!

One small comment on simplifying the way resources are loaded, then LGTM.

@awesominat awesominat force-pushed the add-kubectl-delete-tests branch from fafd4d7 to fc32b50 Compare November 28, 2023 04:26
@brendandburns
Copy link
Contributor

/lgtm
/approve

Thanks for the quick changes!

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 28, 2023
@awesominat
Copy link
Contributor Author

readAllBytes() does not exist in Java 8 (introduced in Java 9), I will look for an alternative.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 28, 2023
@awesominat
Copy link
Contributor Author

verified that the code does compile on Java 8 and 17, should work now

@brendandburns
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 28, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awesominat, brendandburns

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit a57c44e into kubernetes-client:master Nov 28, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yue9944882 yue9944882 Awaiting requested review from yue9944882

@brendandburns brendandburns Awaiting requested review from brendandburns

Assignees

@brendandburns brendandburns

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@awesominat @brendandburns @k8s-ci-robot