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

KIALI-1464 labels need to come from pods not the deployments #485

Merged
merged 1 commit into from Sep 7, 2018

Conversation

@aljesusg
Copy link
Contributor

commented Sep 7, 2018

** Issue reference **

KIALI-1464

@aljesusg aljesusg requested a review from lucasponce Sep 7, 2018
/** Check the labels app and version required by Istio*/
_, workload.AppLabel = d.Labels["app"]
_, workload.VersionLabel = d.Labels["version"]
/** Check the labels app and version required by Istio in tempalte Pods*/

This comment has been minimized.

Copy link
@lucasponce

lucasponce Sep 7, 2018

Contributor

typo "Template"

_, workload.AppLabel = d.Labels["app"]
_, workload.VersionLabel = d.Labels["version"]
/** Check the labels app and version required by Istio in tempalte Pods*/
_, workload.AppLabel = d.Spec.Template.Labels["app"]

This comment has been minimized.

Copy link
@lucasponce

lucasponce Sep 7, 2018

Contributor

As Spec.Template could not exist, it needs some protection.

This comment has been minimized.

Copy link
@aljesusg

aljesusg Sep 7, 2018

Author Contributor

That is why has

 _, workload.AppLabel = d.Spec.Template.Labels["app"]

See test https://github.com/kiali/kiali/pull/485/files#diff-a4042028fcfe2a41d43feb3d77ae37a9L92 No tempalte object and run

This comment has been minimized.

Copy link
@aljesusg

aljesusg Sep 7, 2018

Author Contributor

On the other hand Istio create the object Tempalte with empty values.

Template: v1.PodTemplateSpec{
ObjectMeta: meta_v1.ObjectMeta{
Labels: map[string]string{},
},
},

So the value will be "" for the first parameter _ and for second workload.AppLabel if the key exist or not

@jotak

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2018

Side question - I know this is not the topic of this jira, but... we're hardcoding "app" and "version" strings whereas they can be configured in kiali configmap. Is it expected? Like, do we expect the validation warning to pop up because the user doesn't use exactly "app" and "version", or should we warn because he doesn't use the labels that he defined in kiali config?
I would opt for the latter.

@lucasponce

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2018

we're hardcoding "app" and "version" strings

Good point. Yes in most places this is fetched from the config, I guess it can go in the PR as well.

@aljesusg

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2018

No, it was my fault there are values in the configuration I think that this was done before I am gonna change it. Thanks @jotak

@jotak

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2018

@aljesusg ok, if you change it in this PR then I've seen another place where the same is done: file business/services.go in buildServiceList

@aljesusg

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2018

Changed

@aljesusg

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2018

Wait I am moving all app to config

Copy link
Contributor

left a comment

LGTM
By the way, about the protection of d.Spec.Template.Labels[...] : I think it's ok because none of these fields is a pointer, so they're all zero-valued as empty structs if I'm correct.

@jotak

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2018

@aljesusg I didn't see other places with the same problem ... except maybe in graph but I'm not sure if it's intentional or not ; and in tests but to me it's ok to hardcode in tests.

}

func fakeDeploymentList() *v1beta1.DeploymentList {
conf := config.Get()

This comment has been minimized.

Copy link
@lucasponce

lucasponce Sep 7, 2018

Contributor

did you setup the config on the test ? otherwise this may fail.

This comment has been minimized.

Copy link
@aljesusg

aljesusg Sep 7, 2018

Author Contributor

I see....maybe it's a good moment to move all mocks to the same file ^^

This comment has been minimized.

Copy link
@aljesusg

aljesusg Sep 7, 2018

Author Contributor

Anyway...this config comes from "github.com/kiali/kiali/config" and it's working

This comment has been minimized.

Copy link
@lucasponce

lucasponce Sep 7, 2018

Contributor

There is a separated JIRA for that to be planned in Sprint 11.
But yes, it's some of the areas identified to improve.

@aljesusg aljesusg force-pushed the aljesusg:fix_validation branch from 6315e18 to a98b72e Sep 7, 2018
@aljesusg aljesusg requested review from jotak and lucasponce Sep 7, 2018
@aljesusg aljesusg force-pushed the aljesusg:fix_validation branch from a98b72e to 9a1bf51 Sep 7, 2018
@aljesusg aljesusg removed the do not merge label Sep 7, 2018
@aljesusg aljesusg merged commit c745988 into kiali:master Sep 7, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@aljesusg aljesusg deleted the aljesusg:fix_validation branch Sep 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.