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

make the main endpoint more CRUD #25

Merged
merged 13 commits into from
Aug 8, 2018
Merged

make the main endpoint more CRUD #25

merged 13 commits into from
Aug 8, 2018

Conversation

rokroskar
Copy link
Member

@rokroskar rokroskar commented Jul 11, 2018 •
edited
Loading

  • use POST for launch and GET for server status
  • give some more user-friendly feedback (simple auto-reload in html?)
  • basic use of k8s API to get notebook container status

addresses #24

@rokroskar rokroskar changed the title make the main endpoint more CRUD [WIP] make the main endpoint more CRUD Jul 11, 2018
@rokroskar
make the main endpoint more CRUD
23e4391 
use POST for launch and GET for server status

addresses #24
@rokroskar
add page for showing server status
a5d9653
@rokroskar rokroskar mentioned this pull request Jul 12, 2018
Merged
@rokroskar rokroskar changed the title [WIP] make the main endpoint more CRUD make the main endpoint more CRUD Jul 12, 2018
@rokroskar rokroskar requested a review from leafty July 12, 2018 14:31
@rokroskar
Copy link
Member Author

rokroskar commented Jul 12, 2018 •
edited
Loading

The basic functionality is there -- needs to be extended to handle e.g. failed pod launches, I will make additional issues for it.

leafty
leafty previously requested changes Jul 12, 2018
View reviewed changes
src/notebooks_service.py

# check if we are running on k8s
try:
from kubernetes import client, config
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this test is enough. Running:

config.load_incluster_config()

should fail outside of k8s.
You can also see if paths like /var/run/secrets/kubernetes.io/serviceaccount exists and are not empty.

leafty
leafty reviewed Jul 12, 2018
View reviewed changes
src/notebooks_service.py Outdated
server_name=server_name
)
)
status = 'spawn initialized'
Copy link
Member

Choose a reason for hiding this comment

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

is it intended that we don't return here?

Copy link
Member Author

Choose a reason for hiding this comment

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

oops yes this status is superflous. If we return here, it wouldn't make sense to return the html, so I just log that the spawn has been initiated and when we get to the bottom of the function we return with the html that waits for the server.

leafty
leafty reviewed Jul 12, 2018
View reviewed changes
src/notebooks_service.py Outdated
if r.status_code == 201:
# server is already running
return redirect(notebook_url)
status = get_notebook_container_status(user['name'], server_name)
Copy link
Member

Choose a reason for hiding this comment

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

Is that overwritten?

status = 'spawn initialized'

leafty
leafty reviewed Jul 12, 2018
View reviewed changes
Pipfile Outdated
escapism = "*"
kubernetes = "*"

[dev-packages]

[requires]
python_version = "3.6"
Copy link
Member

Choose a reason for hiding this comment

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

Removed because it's forced by the Dockerfile?

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure actually...

@leafty leafty dismissed their stale review July 13, 2018 06:05

adressed

leafty
leafty previously requested changes Jul 13, 2018
View reviewed changes
src/notebooks_service.py Outdated
user=user, server_name=server_name
status = 'not started'

if request.method == 'POST':
Copy link
Member

Choose a reason for hiding this comment

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

This if block is too long (readability of code)

@leafty leafty force-pushed the refactor-endpoints branch 2 times, most recently from f3fb53b to bc2f7bd Compare July 13, 2018 08:06
@leafty leafty requested a review from jirikuncar July 13, 2018 11:17
@leafty leafty dismissed their stale review July 13, 2018 12:07

addressed

@leafty
Copy link
Member

leafty commented Jul 17, 2018

@jirikuncar Since I authored a sizeable bit too, can I get you to review this? Would help to have another pair of eyes look over this.

@leafty
Copy link
Member

leafty commented Jul 17, 2018

@rokroskar can you check my changes? it's not a small change

jirikuncar
jirikuncar reviewed Jul 17, 2018
View reviewed changes
src/notebooks_service.py Outdated
) as f:
kubernetes_namespace = f.read()
KUBERNETES = True
except:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not use bare except.

jirikuncar
jirikuncar reviewed Jul 17, 2018
View reviewed changes
src/templates/server_status.html
@@ -0,0 +1,22 @@
<!DOCTYPE html>
Copy link
Contributor

Choose a reason for hiding this comment

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

Templates should contain a license header too.

Copy link
Member

Choose a reason for hiding this comment

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

This is currently not the case in renku-ui --> https://github.com/SwissDataScienceCenter/renku-ui/blob/master/public/index.html

Also, I do not see putting a license header on HTML pages as a standard practice. (Check sites like github, docker, gitlab, stack overflow, etc.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Since it's a Jinja2 template you can include it as a comment.

{#
...
#}

Copy link
Member Author

Choose a reason for hiding this comment

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

is it necessary? required? @erbou ?

jirikuncar
jirikuncar reviewed Jul 17, 2018
View reviewed changes
src/notebooks_service.py Outdated
kubernetes_namespace = f.read()
KUBERNETES = True
except:
KUBERNETES = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to start the server without K8S?

rokroskar
rokroskar commented Jul 20, 2018
View reviewed changes
src/notebooks_service.py Outdated
# server is already running
app.logger.debug(
'server {server_name} running'.format(server_name=server_name)
)
return redirect(notebook_url)
Copy link
Member Author

Choose a reason for hiding this comment

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

we don't really need to return here since get_user_server_status will redirect anyway?

@rokroskar
Copy link
Member Author

I think this is gtg @jirikuncar and @leafty -- just waiting on @ableuler to do two minor changes to the UI PR SwissDataScienceCenter/renku-ui#266

leafty
leafty previously approved these changes Jul 23, 2018
View reviewed changes
@rokroskar rokroskar mentioned this pull request Aug 7, 2018
Merged
@rokroskar rokroskar merged commit ae491c4 into master Aug 8, 2018
@rokroskar rokroskar deleted the refactor-endpoints branch August 8, 2018 07:32
@leafty
Copy link
Member

leafty commented Aug 9, 2018

Closes #26

@leafty leafty mentioned this pull request Aug 9, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jirikuncar jirikuncar jirikuncar left review comments

@leafty leafty leafty approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rokroskar @leafty @jirikuncar