Skip to content

Conversation

@wbuchwalter
Copy link
Contributor

@wbuchwalter wbuchwalter commented Jan 19, 2018

Fix #334 & #324.

Adds a selector to choose a namespace in which we should list the TFJobs.
By default we look into every namespace.
screen shot 2018-01-19 at 4 10 56 pm

Also handles namespaces correctly at creation.
If a job is created in a new (non-existant) namespace, the namespace will be created before-hand.


This change is Reviewable

@k8s-ci-robot
Copy link

Hi @wbuchwalter. Thanks for your PR.

I'm waiting for a kubernetes or tensorflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@coveralls
Copy link

coveralls commented Jan 19, 2018

Coverage Status

Coverage decreased (-0.1%) to 31.439% when pulling a6c16d1 on wbuchwalter:dashboard-ns-handling into 2c0f7d4 on tensorflow:master.

@coveralls
Copy link

coveralls commented Jan 19, 2018

Coverage Status

Coverage remained the same at 31.611% when pulling c5bd02b on wbuchwalter:dashboard-ns-handling into 74a958b on tensorflow:master.

import { getTfJobListService } from "../services";
import { getTfJobListService, getNamespaces } from "../services";

const all_namespaces_key = "All namespaces";
Copy link
Member

Choose a reason for hiding this comment

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

camel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, sorry, had a Python moment apparently...

func (apiHandler *APIHandler) handleGetNamespaces(request *restful.Request, response *restful.Response) {
l, err := apiHandler.cManager.ClientSet.CoreV1().Namespaces().List(metav1.ListOptions{})
if err != nil {
panic(err)
Copy link
Member

Choose a reason for hiding this comment

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

maybe also need to handle this?

@jlewi
Copy link
Contributor

jlewi commented Jan 22, 2018

/ok-to-test

@wbuchwalter
Copy link
Contributor Author

wbuchwalter commented Jan 22, 2018

/retest

@jimexist Do you want to give it another look?
@jlewi Airflow seems to consistently fail on the teardown_cluster (while the tests themselves succeed) step, not really sure why.

<AppBar />
<div id="main" style={this.styles.mainStyle}>
<div style={this.styles.list}>
<div>
Copy link
Member

Choose a reason for hiding this comment

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

no need for extra div?

@wbuchwalter
Copy link
Contributor Author

/retest

@wbuchwalter
Copy link
Contributor Author

@jlewi Could you review this? Need approval before merging.

@jimexist
Copy link
Member

LGTM.

I don't have write access yet, but just FYI

@jlewi jlewi merged commit 8caa0c9 into kubeflow:master Jan 25, 2018
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.

5 participants