Skip to content

[Ready to review] Refactoring SMC env variable config #1232

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

Merged
merged 5 commits into from
Jul 23, 2025

Conversation

kumar-shivam-ranjan
Copy link
Member

@kumar-shivam-ranjan kumar-shivam-ranjan commented Jul 21, 2025

Description

This PR is intended to enhance the SMC environment variable configuration for AQUA. After this change, any new environment variable introduced as part of CP config should automatically be picked by ADS.

Related PR

https://bitbucket.oci.oraclecorp.com/projects/ODSC/repos/odsc-pegasus-control-plane/pull-requests/4689/overview

Unit tests

Screenshot 2025-07-22 at 11 37 39 PM

Test results

Was able to successfully deploy granite-vision model with the latest changes in this PR
Screenshot 2025-07-23 at 2 01 24 AM

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jul 21, 2025
Copy link

📌 Cov diff with main:

Coverage-0%

📌 Overall coverage:

Coverage-18.60%

@kumar-shivam-ranjan kumar-shivam-ranjan changed the title [WIP] Refactoring SMC env variable config [Ready to review] Refactoring SMC env variable config Jul 22, 2025
Copy link

📌 Cov diff with main:

Coverage-0%

📌 Overall coverage:

Coverage-18.60%

Copy link

📌 Cov diff with main:

Coverage-0%

📌 Overall coverage:

Coverage-18.60%

for key, default in config_keys.items()
if key in additional_configurations
]
env_vars_dict = json.loads(
Copy link
Member

Choose a reason for hiding this comment

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

For the transition period, I would support both. If we merge it as is and deploy without container changes, this will break the AQUA. We can add TODO indicating that the old logic needs to be deleted after the containers config related changes will be published.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.
Thanks!

Copy link

📌 Cov diff with main:

Coverage-60%

📌 Overall coverage:

Coverage-58.48%

@kumar-shivam-ranjan kumar-shivam-ranjan self-assigned this Jul 23, 2025
@kumar-shivam-ranjan kumar-shivam-ranjan merged commit 373bb69 into main Jul 23, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants