-
Notifications
You must be signed in to change notification settings - Fork 167
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
Fixes warnings in test output (and a potential bug) #261
Conversation
When not passing in a http_proxy_uri into the Kubeclient::Client.new, calling most methods will then cause a flurry of warnings (when -w is enabled) since it never is defined. Setting it explicitly to nil if the http_proxy_uri argument is not passed in will remove this warning.
cc @zakiva |
@@ -17,7 +17,7 @@ class MissingKindCompatibility | |||
'replicationcontrollers' => 'ReplicationController', | |||
'resourcequotas' => 'ResourceQuota', | |||
'secrets' => 'Secret', | |||
'securitycontextconstraints' => 'SecurityContextConstraints', | |||
'securitycontextconstraints' => 'SecurityContextConstraint', |
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.
curling k8s discovery I get
$ curl -k -s -H "Authorization: Bearer $OSH_TOKEN" https://$OSH_HOST:8443/api/v1
{
"name": "securitycontextconstraints",
"namespaced": false,
"kind": "SecurityContextConstraints",
"verbs": [
"create",
"delete",
"deletecollection",
"get",
"list",
"patch",
"update",
"watch"
],
"shortNames": [
"scc"
]
},
The purpose of this is to imitate the discovery so I think this is what we need. Have you any problems caused by this?
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.
oh I see you wrote we define the same method twice. We need to mitigate it like we did for endpoints:
https://github.com/abonas/kubeclient/blob/1e7e4f4db3c3f79985651db298d60f6ca31be778/lib/kubeclient/common.rb#L132
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.
@moolitayer Fair enough, want me to do that then?
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.
Yes, thanks
@@ -73,7 +73,7 @@ def initialize_client( | |||
# Allow passing partial timeouts hash, without unspecified | |||
# @timeouts[:foo] == nil resulting in infinite timeout. | |||
@timeouts = DEFAULT_TIMEOUTS.merge(timeouts) | |||
@http_proxy_uri = http_proxy_uri.to_s if http_proxy_uri | |||
@http_proxy_uri = http_proxy_uri ? http_proxy_uri.to_s : nil |
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.
looks good
When the MissingKindCompatability class was introduced in ManageIQ#214, it the mapping has a plural form of the kind, instead of the singular form like most of the other entries. When this gets loaded in the test/test_resource_list_without_kind.rb, it tries defining the `get_security_context_constraints` twice: * Once for the list action * Once for the record This means that you would not have both a singluar and plural method name for the entities.method_names list, and they both would be plural. To fix, do something similar to what was done for the `Endpoints` entry, which is to have a rule for it in `Kubeclient::Common#parse_definition`
6b13af6
to
850bca4
Compare
Sorry for the delay on addressing your change. Just pushed that now. |
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.
LGTM 👍
ManageIQ should be unaffected by the bug mentioned here as we don't fetch securitycontextconstraints |
Honestly, I was just annoyed by all of the extra warning output 😄 |
😄 That's our gain @NickLaMuro |
Fixes warnings in test output (and a potential bug)
Fixes warnings in test output (and a potential bug)
These warnings have actually been in place for a while, but because of the recent pull request to update dependencies, rake was updated from version 10 to version 12, it included this PR to run with
-w
, which will print out warnings when the test suite is executed.Warnings Fixed
instance variable @http_proxy_uri not initialized
http_proxy_uri
arg is not passed into theKubernetes::Client.new
, and any method using@http_proxy_uri
is called (which is most useful methods on the client.warning: method redefined; discarding old get_security_context_constraints
Kubeclient::Common::MissingKindCompatability
class, which the'securitycontextconstraints' => 'SecurityContextConstraint',
map was setup with the plural version of the "kind" (SecurityContextConstraints
), instead of the Singular. This causedget_security_context_constraints
to be defined for both theget_entity
and theget_entities
actions, but only theget_entity
action would be available.The second one is probably a bug that has existed since #214, so the
v1.0.0
release and newer (I think) ofkubeclient
have been unable to access thesecuritycontextconstraints
index actions on older versions of the kubernetes API. UsingKubeclient::Client#get_security_constant_contraints
would actually be running theget
action for a single record.The above needs to be confirmed, but I relatively sure this would be the case (unfortunately I don't really have a live and legacy kubernetes environment where I could test this).