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

Implement usage of nested APIs #457

Merged
merged 1 commit into from
Aug 28, 2020
Merged

Implement usage of nested APIs #457

merged 1 commit into from
Aug 28, 2020

Conversation

T4cC0re
Copy link
Contributor

@T4cC0re T4cC0re commented Aug 12, 2020

handle_uri expected the Kubernetes API to be rooted at / if more than 2
components were included in the provided URL. This was done to extract a
potential api_group from the URL.

This commit changes that behaviour, to parse that api_group via a regex,
rather than leaning on a strict URL pattern.
This commit also takes into account, that OpenShift uses /oapi, as well
as including a test-suite for that new API URL handling.

Implements #318
Fixes #418

@T4cC0re
Copy link
Contributor Author

T4cC0re commented Aug 12, 2020

This would also yield to fix https://gitlab.com/gitlab-org/gitlab/-/issues/22043

@cben
Copy link
Collaborator

cben commented Aug 17, 2020

This is awesome and very important, thanks a lot for tackling this! ❤️
Thanks especially for the detailed tests.

  • The /api/but/I/want/a/hidden/k8s/api cases are excellent subtle corner case to check 👏
  • Let's add a test for /k8s/clusters/c-somerancherID/apis/this_is_the_group for complete coverage of Rancher-style URLs.
    (I expect it works if api/but/I/want/a/hidden/k8s/apis/this_is_the_group test works, but the more the merrier)
  • What these tests are checking is that the final re-assembled url is correct.
    Can you also check that @api_group and @api_version instance vars are correct?
    I believe in some cases the url was already correct but wrong @api_group and @api_version caused create operations to fail (due to sending non-existent apiVersion).
  • 'v1' is already the default, so let's use something else in tests e.g. 'v2' to confirm it's respected.
    (I'm certain it is, default is only when omitting 2nd arg entirely, but still would make a stronger test)

@T4cC0re
Copy link
Contributor Author

T4cC0re commented Aug 20, 2020

Thanks for those pointers. I will take a look at that :)

@T4cC0re
Copy link
Contributor Author

T4cC0re commented Aug 25, 2020

@cben I just force-pushed an amended commit, that includes the tests you asked for. Would you mind reviewing it? :)

@T4cC0re
Copy link
Contributor Author

T4cC0re commented Aug 25, 2020

Just saw, that rubocop failed. Will fix that real quick

`handle_uri` expected the Kubernetes API to be rooted at `/` if more than 2
components were included in the provided URL. This was done to extract a
potential `api_group` from the URL.

This commit changes that behaviour, to parse that `api_group` via a regex,
rather than leaning on a strict URL pattern.
This commit also takes into account, that OpenShift uses `/oapi`, as well
as including a test-suite for that new API URL handling.
Tests also include checks for the `@api_group` and `@api_version` instance
variables and a non-default version.

Implements ManageIQ#318
Fixes ManageIQ#418
Copy link
Collaborator

@cben cben left a comment

Choose a reason for hiding this comment

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

Perfect 💯
Thanks again!

client = Kubeclient::Client.new('http://localhost:8080/apis/this_is_the_group', 'v1')
rest_client = client.rest_client
assert_equal('v1', client.instance_variable_get(:@api_version))
assert_equal('this_is_the_group/', client.instance_variable_get(:@api_group))
Copy link
Collaborator

Choose a reason for hiding this comment

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

The trailing slash looked surprising to me, but yep, we had it before and we're sending
hash[:apiVersion] = @api_group + @api_version
so this is good ✔️.
(Also this representation is convenient for automatically handling both foo/v1 and core v1)

@cben cben merged commit 96bd0a3 into ManageIQ:master Aug 28, 2020
@cben
Copy link
Collaborator

cben commented Aug 28, 2020

I'll release this by Sunday.

cben added a commit to cben/kubeclient that referenced this pull request Aug 31, 2020
cben added a commit to cben/kubeclient that referenced this pull request Aug 31, 2020
@cben
Copy link
Collaborator

cben commented Aug 31, 2020

Well, for a sufficiently late definition of "Sunday". Released 4.9.1.

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.

Request K8s API, But it return bad-request (400)
2 participants