-
Notifications
You must be signed in to change notification settings - Fork 111
Feature/314 namespace as resource kind #322
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
Feature/314 namespace as resource kind #322
Conversation
…ed ResourceKind for Namespace and registered it in the ResourceFactory.
@Override | ||
public <T extends IResource> T create(T resource) { | ||
return create(resource, resource.getNamespace()); | ||
return create(resource, resource.getNamespaceName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adietish This is an API change that seems significant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Obirah nice!
@jcantrill nicely spotted!
I'd ack this as change as preferrable to any kind of more compatible change (ex. call this #getNamespaceResource and and keep the existing #getNamespace) since it's consistent with the existing naming schemes (ex. #getProject retuns IProject, not the project name). Even more since we bumped to the next major lately (6.0.0).
Thoughts?
[test] |
Evaluated for javaclient test up to 59058dd |
Openshift Restclient Java Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test-openshift-restclient-java/374/) (Base Commit: f78e598) (PR Branch Commit: 59058dd) |
@jcantrill @jeffmaury are you okay merging this breaking change (it will require a fix in JBoss Tools and all other consumers) for the reasons mentioned above? #322 (comment) |
@adietish I defer to if you desire this change. The PR otherwise lgtm |
@jcantrill ok great, let me prepare the PR for JBoss Tools and merge this as soon as I have it ready. |
[merge] |
Evaluated for javaclient merge up to 59058dd |
Openshift Restclient Java Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test-openshift-restclient-java/375/) (Base Commit: f78e598) (PR Branch Commit: 59058dd) |
artifact deployed to maven repository at jboss.org |
No description provided.