-
Notifications
You must be signed in to change notification settings - Fork 998
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
Add helm charts for feast jobservice #1081
Conversation
Signed-off-by: Tsotne Tabidze <[email protected]>
infra/charts/feast/charts/feast-jobservice/templates/deployment.yaml
Outdated
Show resolved
Hide resolved
Signed-off-by: Tsotne Tabidze <[email protected]>
@oavdeev thanks for the comments, I realized most of the clean up stuff and was cleaning up the diff. Didn't find an equivalent functionality of Phabricator's "Plan Changes" to not notify reviewers... Let me know if there's anything else left! |
Signed-off-by: Tsotne Tabidze <[email protected]>
627afa4
to
4d7c9e4
Compare
{{- toYaml . | nindent 8 }} | ||
{{- end }} | ||
|
||
{{- if .Values.gcpServiceAccount.enabled }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to hardcode the GCP specific fields here still? I think we want to move away from this (if we support AWS)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@woop we should probably handle that in another PR to get this one unblocked.
- All services (core, serving, jupyter) have the gcp-specific fields, so we need to figure out the overall solution here, not just for job service
- Looks like for AWS @oavdeev wrote the terraform script that uses envOverrides to override some environment variables, and he won't need to hardcode any AWS-specific stuff. However, this approach can't be used readily, since we don't have terraform for GCP deployment & some of these gcp fields are used outside env variables right now. Therefore I don't have a clear alternative to move these variables to.
- It won't affect AWS deployment in the short term, so we can temporarily leave it here
Let me know what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, lets go ahead with cleaning this up in another PR.
Signed-off-by: Tsotne Tabidze <[email protected]>
4e9310b
to
d6adf2e
Compare
Signed-off-by: Tsotne Tabidze <[email protected]>
3b78bc8
to
b3aca8d
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: oavdeev, tsotnet The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
Signed-off-by: Tsotne Tabidze <[email protected]>
0532ae1
to
e6c0887
Compare
Signed-off-by: Tsotne Tabidze <[email protected]>
a8336bd
to
5ad7a3d
Compare
/retest |
1 similar comment
/retest |
What this PR does / why we need it: This PR adds Helm Chart for Feast Job Service. Tested locally on minikube.