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

Use REST resource mapper #85

Merged
merged 3 commits into from
Jan 12, 2023
Merged

Use REST resource mapper #85

merged 3 commits into from
Jan 12, 2023

Conversation

toddtreece
Copy link
Member

@toddtreece toddtreece commented Jan 10, 2023

Why

This is an attempt to add support for custom resources.

Related issues:

What

@toddtreece toddtreece marked this pull request as draft January 10, 2023 13:13
@pablochacin pablochacin self-requested a review January 10, 2023 13:25
pkg/api/api.go Show resolved Hide resolved
pkg/resources/resources.go Outdated Show resolved Hide resolved
@toddtreece toddtreece marked this pull request as ready for review January 11, 2023 04:44
@pablochacin
Copy link
Contributor

Hi @toddtreece Thanks for the PR. The changes look good to me.

@javaducky
Copy link
Contributor

Awesome @toddtreece ! I'm just taking a look at it now.

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.

Please update examples/ingress_operations.js as this is now broken. Get, List, and Delete operations now require a fully-qualified resource type (Ingress.networking.k8s.io versus Ingress).

Sorry that we have a ton of cruft in the examples directory...many can be ignored as we will be replacing most of them; ingress_operations.js is a newer one that I knew should be working. We'll be getting full integration tests set up in the future.

💪 I was able to install a CRD (K6 type from k6-operator) and perform CRUD operations successfully as well.

@pablochacin
Copy link
Contributor

pablochacin commented Jan 12, 2023

I was able to install a CRD (K6 type from k6-operator) and perform CRUD operations successfully as well.

yay! It would be nice to add an example of using CRDs 🤔 I'm creating and issue for that.

Sorry that we have a ton of cruft in the examples directory...many can be ignored as we will be replacing most of them;

@javaducky maybe it is time to address this issue. Once we have a clear list of examples we want to maintain, I have some ideas of how to use them to implement #48 based on what I have been doing in xk6-disruptor.

@pablochacin pablochacin changed the title Add support for custom resources Use REST resource mapper Jan 12, 2023
@pablochacin
Copy link
Contributor

@toddtreece I renamed the PR because the title was misleading. Even when the motivation of the PR is to support CRDs the scope is basically introduce the REST mapper. If the scope were to introduce support for CRDs then we should had to add examples and tests for that, what we haven't done in this PR.

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.

LGTM! Thank you!

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.

3 participants