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.

Sign up for GitHub

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

Jump to bottom

Implement GraphQL query for pods #2280

Merged
merged 4 commits into from Jan 17, 2019
Merged

Implement GraphQL query for pods #2280

merged 4 commits into from Jan 17, 2019

Conversation

tgorgol
Copy link
Contributor

@tgorgol tgorgol commented Jan 14, 2019 •
edited

Description

Changes proposed in this pull request:

  • Create query for pods within k8s domain in UI API Layer

Related issue(s)

#2000

@tgorgol tgorgol added kind/feature Categorizes issue or PR as related to a new feature. area/busola Related to all activities around the Busola UI and all its views labels Jan 14, 2019
@tgorgol tgorgol self-assigned this Jan 14, 2019
magicmatatjahu
magicmatatjahu reviewed Jan 16, 2019
View reviewed changes
Copy link
Contributor

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

LGTM, but please refer to my comments.

components/ui-api-layer/internal/domain/k8s/pod_converter.go Outdated
return nil, nil
}

containerStates := c.containerStatusesToGQLContainerStates(in.Status.ContainerStatuses)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe would be better if all logic for extracting statuses would be in separate package, eg. k8s/status/pod.go. What do you think about that @michal-hudy?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I agree 😄

components/ui-api-layer/internal/domain/k8s/pod_converter.go Outdated Show resolved Hide resolved
components/ui-api-layer/internal/domain/k8s/pod_converter.go Outdated Show resolved Hide resolved
components/ui-api-layer/internal/domain/k8s/pod_converter.go Outdated Show resolved Hide resolved
components/ui-api-layer/internal/domain/k8s/pod_converter.go Outdated Show resolved Hide resolved
tests/ui-api-layer-acceptance-tests/internal/domain/k8s/pod_test.go Outdated Show resolved Hide resolved
components/ui-api-layer/internal/domain/k8s/pod_converter.go
return c.getWaitingContainerState(nil)
}

func (c *podConverter) podToGQLJSON(in *v1.Pod) (gqlschema.JSON, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably name podSpecificToGQLJSON would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't that suggest only the PodSpec field of Pod is being converted while we want to convert whole Pod?

hudymi
hudymi reviewed Jan 16, 2019
View reviewed changes
components/ui-api-layer/internal/domain/k8s/pod_converter.go Outdated
return containerStates
}

func (c *podConverter) getWaitingContainerState(in *v1.ContainerStateWaiting) gqlschema.ContainerState {
Copy link
Contributor

Choose a reason for hiding this comment

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

Export logic responsible for extracting status to separate package, please see how it is created in servicecatalog domain.

components/ui-api-layer/internal/domain/k8s/pod_converter.go Outdated
var reason, message *string

if in.Reason != "" {
tmp := in.Reason
Copy link
Contributor

Choose a reason for hiding this comment

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

tmp is not needed. Fix also in other places 😄

components/ui-api-layer/internal/domain/k8s/pod_converter.go Outdated
}
}

var reason, message *string
Copy link
Contributor

Choose a reason for hiding this comment

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

Why reason and message are pointers?

components/ui-api-layer/internal/domain/k8s/pod_converter.go Outdated

jsonByte, err := json.Marshal(in)
if err != nil {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

Add context

components/ui-api-layer/internal/domain/k8s/pod_converter.go Outdated
var jsonMap map[string]interface{}
err = json.Unmarshal(jsonByte, &jsonMap)
if err != nil {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

Add context, also in other places

components/ui-api-layer/internal/gqlschema/schema.graphql Outdated

type ContainerState {
state: ContainerStateType!
reason: String
Copy link
Contributor

Choose a reason for hiding this comment

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

Return empty string if reason or message is not provided. Kubernetes API also returns empty string

    reason: String!
    message: String!

components/ui-api-layer/internal/gqlschema/schema.graphql Show resolved Hide resolved
hudymi
hudymi reviewed Jan 17, 2019
View reviewed changes
Copy link
Contributor

@hudymi hudymi left a comment

Choose a reason for hiding this comment

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

Small changes 😄

components/ui-api-layer/internal/domain/k8s/status/pod.go Outdated

type PodExtractor struct{}

func (ext *PodExtractor) ContainerStatusesToGQLContainerStates(in []v1.ContainerStatus) []gqlschema.ContainerState {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm... this is state extractor, not status ;) My bad, I suggested moving it to status.

So move it to state package and rename method to States and it will be ContainerExtractor :)

components/ui-api-layer/internal/domain/k8s/status/pod.go Outdated
type PodExtractor struct{}

func (ext *PodExtractor) ContainerStatusesToGQLContainerStates(in []v1.ContainerStatus) []gqlschema.ContainerState {
containerStates := []gqlschema.ContainerState{}
Copy link
Contributor

Choose a reason for hiding this comment

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

You already know the size of the slice

containerStates := make([]gqlschema.ContainerState, len(in))

tests/ui-api-layer-acceptance-tests/internal/domain/k8s/pod_test.go Outdated

type containerState struct {
State containerStateType `json:"state"`
Reason *string `json:"reason"`
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not a pointer

hudymi
hudymi approved these changes Jan 17, 2019
View reviewed changes
Copy link
Contributor

@hudymi hudymi left a comment

Choose a reason for hiding this comment

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

LGTM

@tgorgol tgorgol merged commit 339a2e6 into kyma-project:master Jan 17, 2019
@tgorgol tgorgol deleted the add-graphql-pods-query branch January 17, 2019 12:16
grischperl pushed a commit to grischperl/kyma that referenced this pull request Nov 10, 2020
@dekiel
readme update (kyma-project#2280)
3a9e905 
* readme update

* Update development/README.md

Co-Authored-By: Maja Kurcius <maja.kurcius@sap.com>

* Update development/README.md

Co-Authored-By: Maja Kurcius <maja.kurcius@sap.com>

Co-authored-by: Maja Kurcius <maja.kurcius@sap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@magicmatatjahu magicmatatjahu magicmatatjahu approved these changes

@hudymi hudymi hudymi approved these changes

@adamwalach adamwalach Awaiting requested review from adamwalach

@aerfio aerfio Awaiting requested review from aerfio

@akucharska akucharska Awaiting requested review from akucharska

@aszecowka aszecowka Awaiting requested review from aszecowka

@jasiu001 jasiu001 Awaiting requested review from jasiu001

@mjasinski5 mjasinski5 Awaiting requested review from mjasinski5

@mszostok mszostok Awaiting requested review from mszostok

@piotrmiskiewicz piotrmiskiewicz Awaiting requested review from piotrmiskiewicz

@PK85 PK85 Awaiting requested review from PK85

@pkosiec pkosiec Awaiting requested review from pkosiec

@polskikiel polskikiel Awaiting requested review from polskikiel

Assignees

@magicmatatjahu magicmatatjahu

@hudymi hudymi

@tgorgol tgorgol

Labels
area/busola Related to all activities around the Busola UI and all its views kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tgorgol @magicmatatjahu @hudymi