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

Update the rancher dynamic client to use its concrete struct types #31

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

git-ival
Copy link
Contributor

@git-ival git-ival commented Jan 9, 2024

Issue:

Problem

As a developer attempting to debug issues while using the dynamic client, it was difficult to follow what code implemented the underlying functions. This was due to the dynamic client functions returning the upstream k8s dynamic client types instead of the concrete ones of the rancher dynamic client.

Solution

Update the dynamic client and other references to it so that they reference the concrete rancher dynamic client types.

Testing

Was able to build locally, have not run any tests from the rancher/rancher repo that might utilize the dynamic client. Custom tests utilizing the rancher dynamic client were able to run successfully and made debugging a bit easier.

Engineering Testing

Manual Testing

Automated Testing

QA Testing Considerations

Regressions Considerations

There should not be regressions as this is a minor change that updates the types to the concrete ones provided in the rancher dynamic client code.

Existing / newly added automated tests that provide evidence there are no regressions: N/A

Copy link
Member

@caliskanugur caliskanugur left a comment

Choose a reason for hiding this comment

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

Looks solid, I've got a few questions and a naming nit

@git-ival git-ival force-pushed the update-dynamic-client branch from 5273d52 to a69be5a Compare February 5, 2024 19:41
@git-ival git-ival requested a review from caliskanugur February 5, 2024 21:01
@git-ival
Copy link
Contributor Author

@caliskanugur Anything else needed on my end before this can be reviewed?

@git-ival git-ival force-pushed the update-dynamic-client branch from dd44042 to ab92c36 Compare March 5, 2024 23:47
@git-ival git-ival added the automation-enhancement To indicate this is related to enhancements in code related to automation label Mar 14, 2024
Copy link
Contributor

@igomez06 igomez06 left a comment

Choose a reason for hiding this comment

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

LGTM I think this looks good, we'll need to merge this as separate version bump, just to make sure there aren't any regressions in rancher.

@git-ival git-ival mentioned this pull request Apr 12, 2024
@git-ival git-ival force-pushed the update-dynamic-client branch 2 times, most recently from 9f9566e to dd1fea2 Compare April 18, 2024 22:52
@git-ival git-ival removed the request for review from floatingman May 22, 2024 18:40
@git-ival git-ival force-pushed the update-dynamic-client branch from dd1fea2 to 242ed8b Compare July 5, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation-enhancement To indicate this is related to enhancements in code related to automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants