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

fix(image-builder): Add kaniko api server env vars config to pred job image builder #622

Merged

Conversation

deadlycoconuts
Copy link
Contributor

@deadlycoconuts deadlycoconuts commented Jan 2, 2025

Description

Previously in https://github.com/caraml-dev/merlin/pull/621/files, a change has been made to allow the API server to propagate environment variables within its environment to the build environment of all Kaniko jobs that it spins up. However, this only happens for all Merlin web service image building jobs (and not batch prediction image building jobs) because the config KanikoAPIServerEnvVars is not passed as a field of the batch prediction image building job config.

This PR addresses this bug by simply adding the KanikoAPIServerEnvVars field as one of the new fields to create when the batch prediction image building job config gets initiliased.

Modifications

  • api/cmd/api/setup.go - Add KanikoAPIServerEnvVars to batching prediction image building job config

Tests

None

Checklist

  • Added PR label
  • Added unit test, integration, and/or e2e tests
  • Tested locally
  • Updated documentation
  • Update Swagger spec if the PR introduce API changes
  • Regenerated Golang and Python client if the PR introduces API changes

Release Notes

NONE

@deadlycoconuts deadlycoconuts self-assigned this Jan 2, 2025
@deadlycoconuts deadlycoconuts added the bug Something isn't working label Jan 2, 2025
@deadlycoconuts deadlycoconuts changed the title fix(api): Add kaniko api server env vars config to pred job image builder fix(image-builder): Add kaniko api server env vars config to pred job image builder Jan 2, 2025
Copy link
Contributor

@bthari bthari left a comment

Choose a reason for hiding this comment

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

Thank you for spotting this, LGTM!

@deadlycoconuts
Copy link
Contributor Author

Thanks for the quick review! Haha merging this now!

@deadlycoconuts deadlycoconuts merged commit 50f3887 into main Jan 2, 2025
33 of 34 checks passed
@deadlycoconuts deadlycoconuts deleted the add_kaniko_api_server_env_vars_to_pred_job_builder branch January 2, 2025 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants