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-1347 Add type in workloadEndpoint Details and validations by Workload #433

Merged
merged 1 commit into from Aug 17, 2018

Conversation

aljesusg
Copy link
Collaborator

** Describe the change **

Added the type to the Workload Details Endpoint

worloaddetail

Added Endpoint to check labels and IstioSidecar Validations related a Workload

worklaod_validation

Fake validation

fake_validation
** Issue reference **

KIALI-1347

** Backwards incompatible? **
Yes

doc.go Outdated
@@ -216,6 +223,10 @@ type NamespaceValidations map[string]TypedIstioValidations
// swagger:model
type TypedIstioValidations map[string]NameIstioValidation

// List of validations grouped by workload
// swagger:model
type WorkloadValidations map[string]models.IstioValidation
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just wondering, as just below there's a type defined "List of validations grouped by object name" : wouldn't it be like a more generic thing for the one you're creating? Can it be reused, from a logical point of view? (technically it's the same, but logically I don't know if it makes sense)

@lucasponce lucasponce added the do not merge A PR is not ready to merge label Aug 16, 2018
@lucasponce
Copy link
Contributor

Added Do Not Merge label.
We need some order on these PRs as first we need to include and consolidate some of @xeviknal.
And those also depends on UI PR 555.

validations := make([]*models.IstioCheck, 0)
valid := true
if _, appLabel := pod.Labels["app"]; !appLabel {
//do something here
Copy link
Contributor

Choose a reason for hiding this comment

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

you can probably remove this comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hahaha Yeah sorry xD

}

if _, versionLabel := pod.Labels["version"]; !versionLabel {
//do something here
Copy link
Contributor

Choose a reason for hiding this comment

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

you can probably remove this comment?

pod.Parse(checker.Pod)
validations := make([]*models.IstioCheck, 0)
valid := true
if _, appLabel := pod.Labels["app"]; !appLabel {
Copy link
Contributor

Choose a reason for hiding this comment

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

String "app" should not be hard-coded but taken from config (AppLabelName)

validations = append(validations, &check)
}

if _, versionLabel := pod.Labels["version"]; !versionLabel {
Copy link
Contributor

Choose a reason for hiding this comment

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

String "version" should not be hard-coded but taken from config (VersionLabelName)

@@ -65,6 +65,23 @@ func (in *IstioValidationsService) GetNamespaceValidations(namespace string) (mo
return models.NamespaceValidations{namespace: runObjectCheckers(objectCheckers)}, nil
}

func (in *IstioValidationsService) GetWorkloadValidations(namespace string, workload string) (models.IstioValidations, error) {
selector, err := in.k8s.GetDeploymentSelector(namespace, workload)
Copy link
Contributor

Choose a reason for hiding this comment

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

very good!
Workload may not be a deployment, but we don't care much about that yet. So that's fine

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should refactor this and the Workload model with the new objects in the future

@aljesusg
Copy link
Collaborator Author

@jotak comments addressed, thanks

@lucasponce
Copy link
Contributor

@aljesusg this needs a rebase before to give it a final review. Thx

@aljesusg
Copy link
Collaborator Author

Rebased

Copy link
Contributor

@lucasponce lucasponce left a comment

Choose a reason for hiding this comment

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

LGTM

@lucasponce lucasponce merged commit c0a0737 into kiali:master Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge A PR is not ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants