get rest api to work with 4.0 api structure, also took out oapi refer… #88
Conversation
openshift-ci-robot
added
the
size/M
|
Please note that this fix applies to |
|
I have created a v3 branch but already merged your other PR. And it was filed against |
|
@akostadinov thanks...I think it should be ok, since we need to maintain both v3 and v4 per your discussion. #87 should still apply to |
…ences since it has become obsolete
|
@akostadinov ok, I've resolved the conflicts |
lib/rest_openshift.rb
Outdated
| return perform(**base_opts, method: "DELETE") | ||
| end | ||
|
|
||
| def self.list_projects(base_opts, opts) | ||
| populate("/projects", base_opts, opts) | ||
| def self.list_projects(base_opts, opts, type="project") |
There was a problem hiding this comment.
I think that type here should not be a parameter but hardcoded when calling populate method. Why would we ever want to specify a different type when calling list_projects?
There was a problem hiding this comment.
yeah, we can put it here or at the populate method as you suggested
There was a problem hiding this comment.
Ok, please remove this parameter type from here as well other individual calls. populate looks good.
See my other comment. I would leave it to you about merging k8s and openshift calls in same place.
There was a problem hiding this comment.
@akostadinov ok, I fixed that now.
openshift-ci-robot
added
the
approved
| return Http.request(**base_opts, method: "GET") | ||
| end | ||
|
|
||
| def self.post_role_oapi(base_opts, opts) |
There was a problem hiding this comment.
@pruan-rht any replacement for this method?
There was a problem hiding this comment.
@stuartchuan, for 4.0, oapi has been removed, that's why I took out this method
| def self.populate(path, base_opts, opts) | ||
| populate_common("/oapi/<oapi_version>", path, base_opts, opts) | ||
| def self.populate(path, base_opts, opts, type) | ||
| populate_common("/apis/#{type}.openshift.io/<api_version>", path, base_opts, opts) |
There was a problem hiding this comment.
Using api_version here is not correct. In fact all these something.openshift.io APIs may use different API version. But since it is less broken than before, we can merge as is to get something working. We need more investigation to figure out what might be an approach that makes sense.
/lgtm
There was a problem hiding this comment.
What do you mean by may use different API version? Shouldn't they all be using the same api version?
There was a problem hiding this comment.
well initial assumpion was that there will be one global API version like 1, 2, 3. Now they have multiple something.k8s.io and somehting.openshift.io APIs and each of them can have a different independent API level. Having a single parameter for this will not work.
On the other hand it can be overridden for each call so maybe will work. As long as it works now, I'm fine.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akostadinov, pruan-rht 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 |
…ences since it has become obsolete. According to the upstream documentation there are still differences between
k8sandopenshiftin term of base URL. Introducetypeto use to build the url to fetch from.