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

Add harvester Client #351

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

slickwarren
Copy link
Contributor

@slickwarren slickwarren commented Nov 25, 2024

Issue:

rancher/qa-tasks#468

Problem

currently, we don't have a way to write automation for harvester using the framework.

Solution

harvester's API is based on rancher's steve. In this PR I've introduced a harvester client that wraps steve and catalog clients, introduced a new harvesterConfig, similar to how rancherClient is setup.

We played around with wrapping Steve's API/client directly, but since harvester will need the catalog client as well, giving harvester its own client makes more sense in the long run + to support any use cases their team may have.

Also played around with simplifying the copy-paste logic between rancher and now harvester clients. But this may make more sense to do if we have to introduce another client that uses most of rancher's API which I don't know of a product that would need it tbh. If this approach were taken, the code would be refactored instead of net-new, risking stuff breaking.

Testing

see linked rancher PR; we are now able to use harvester's API and import an existing harvester cluster into an existing rancher setup.

Regressions Considerations

TODO

this is a new client, so no risk of regression.

@slickwarren slickwarren force-pushed the cwarren/harvester-client branch 2 times, most recently from 3c71a80 to 3e4b144 Compare November 25, 2024 19:06
@slickwarren slickwarren marked this pull request as ready for review November 25, 2024 19:11
@slickwarren slickwarren force-pushed the cwarren/harvester-client branch from 3e4b144 to e44a6b6 Compare November 25, 2024 19:28
@slickwarren slickwarren requested a review from a team December 2, 2024 17:29
Copy link
Contributor

@lscalabrini01 lscalabrini01 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@floatingman floatingman left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM, do we need backports for this PR or just main?

@slickwarren
Copy link
Contributor Author

yes we will need backports, I wanted to wait for your review to open them 👍🏼 I'll do that now.

removing unnecessary functions from the harvester client
@slickwarren slickwarren force-pushed the cwarren/harvester-client branch from e44a6b6 to da2238c Compare December 5, 2024 22:41
@caliskanugur caliskanugur merged commit d62d56f into rancher:main Dec 5, 2024
1 check passed
@slickwarren slickwarren deleted the cwarren/harvester-client branch December 6, 2024 17:07
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.

5 participants