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

Libsonnet cleanup #431

Merged
merged 15 commits into from Mar 16, 2018
Merged

Libsonnet cleanup #431

merged 15 commits into from Mar 16, 2018

Conversation

kkasravi
Copy link
Contributor

@kkasravi kkasravi commented Mar 15, 2018

/assign @ankushagarwal

fixes #417


This change is Reviewable

local ambassadorImage = "quay.io/datawire/ambassador:0.26.0",
service:: {
service(serviceType): {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be service(serviceType)::

local diskNames = util.toArray(disks),
local kubeSpawner = $.parts(namespace).kubeSpawner(jupyterHubAuthenticator, diskNames),
local cm = if std.length(diskNames) == 0 then
$.parts(namespace).jupyterHubConfigMapWithoutVolumes
Copy link
Contributor

Choose a reason for hiding this comment

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

jupyterHubConfigMapWithoutVolumes takes a parameter named spawner, but here you are not passing any parameter to jupyterHubConfigMapWithoutVolumes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

both parts of the if logic return a function which is then called with spawner on line # 24.
Original logic is similar.

Copy link
Contributor

Choose a reason for hiding this comment

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

The first part of the if logic returns a function which takes spawner as a parameter, but the second part $.parts(namespace).jupyterHubConfigMapWithVolumes(diskNames) returns an object, not a function.

@kkasravi
Copy link
Contributor Author

@ankushagarwal the logic related to jupyterHubConfigMapWithVolumes was never used or inserted. Instead params.disks is inserted via kubeSpawner and becomes part of jupyterhub_spawner.py.

@ankushagarwal
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ankushagarwal

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 8b48d28 into kubeflow:master Mar 16, 2018
@kkasravi kkasravi deleted the libsonnet_cleanup branch March 16, 2018 17:15
yanniszark pushed a commit to arrikto/kubeflow that referenced this pull request Nov 1, 2019
* AddPytorch operator manifests changes

* Rebase from master

* Removing v1beta2 run from workflow
elenzio9 pushed a commit to arrikto/kubeflow that referenced this pull request Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

normalize libsonnets so their "all(params)" is only called from kubeflow/core/all.libsonnet
3 participants