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 dynamic client for labels #782

Merged
merged 6 commits into from
Jul 9, 2018

Conversation

r2d4
Copy link
Contributor

@r2d4 r2d4 commented Jul 1, 2018

Fixes #708

}

return err
Copy link
Contributor

Choose a reason for hiding this comment

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

return nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

if m.Labels == nil {
m.Labels = map[string]string{}
func addLabels(labels map[string]string, accessor metav1.Object) {
for k, v := range constants.Labels.DefaultLabels {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we apply DefaultLabels to objLabels first and then apply labels?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@nkubala nkubala mentioned this pull request Jul 9, 2018
@r2d4
Copy link
Contributor Author

r2d4 commented Jul 9, 2018

@nkubala PTAL

client, err := kubernetes.Client()
dynClient, err := kubernetes.DynamicClient()
if err != nil {
logrus.Warnf("error retrieving kubernetes client: %s", err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/kubernetes/dynamic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

break // we should only ever apply one patch, so stop here

for _, r := range resources.APIResources {
if r.Kind == gvk.Kind {
Copy link
Contributor

Choose a reason for hiding this comment

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

when I hacked on this I had trouble converting the kind string into something that could be treated as a resource, mostly with capitalization (i.e. pod needs to be converted to Pod, etc.). not sure if this is something you ran into here, but might be worth addressing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I didn't have trouble with that. The only thing that seemed a little odd was that the dynamic client needed a non-empty namespace and parsing the kubeconfig uses "" instead of "default"

Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

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

LGTM assuming tests pass

@r2d4 r2d4 merged commit 0c6d598 into GoogleContainerTools:master Jul 9, 2018
@r2d4 r2d4 deleted the dynamic-programming branch July 9, 2018 21:07
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.

4 participants