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

feat(component): Add apps component jupyter-web-app #125

Merged
merged 2 commits into from
Jan 18, 2022

Conversation

rohank07
Copy link
Contributor

@rohank07 rohank07 commented Jan 18, 2022

Signed-off-by: rohank07 rohank_17@hotmail.ca

@rohank07 rohank07 requested a review from sylus January 18, 2022 16:05
@rohank07 rohank07 linked an issue Jan 18, 2022 that may be closed by this pull request
@sylus
Copy link
Member

sylus commented Jan 18, 2022

@rohank07 @zachomedia

Doesn't the cluster role(s) come from Kubeflow are we performing adjustments on it?
If we are already providing a deployment.yaml why are we doing a patch as well?

@rohank07
Copy link
Contributor Author

rohank07 commented Jan 18, 2022

It appears we're modifying the cluster roles.
Manifest on the left is the upstream cluster role. Manifest on the right is the strategic merged cluster role
image

@sylus
Copy link
Member

sylus commented Jan 18, 2022

@zachomedia in the example above should we own the cluster roles or just apply our deltas via a strategic merge?

@sylus
Copy link
Member

sylus commented Jan 18, 2022

Overall this looks great @rohank07 just checking one thing and then will merge it.

@sylus
Copy link
Member

sylus commented Jan 18, 2022

Hey @rohank07 I might be wrong but I dont see a difference in output when commenting out:

# using a valueFrom instead of value
patches:
- target:
    kind: Deployment
    name: jupyter-web-app-deployment
  patch: |-
    - op: remove
      path: /spec/template/spec/containers/0/env/1/value
    - op: remove
      path: /spec/template/spec/containers/0/env/2/value

@sylus
Copy link
Member

sylus commented Jan 18, 2022

Hey @rohank07 i made a few tiny adjustments but the end manifest is still the same.

Are you ok if i merge this?

@rohank07
Copy link
Contributor Author

Okay to merge! Yeah, no differences in the patch 😅

@sylus sylus merged commit ade2e69 into main Jan 18, 2022
@sylus sylus deleted the feat-apps-jupyter-web-app branch January 18, 2022 20:54
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.

Apps component: jupyter-web-app
2 participants