Skip to content

Conversation

@samhita-alla
Copy link
Contributor

@samhita-alla samhita-alla commented Jun 18, 2025

Tracking issue

Why are the changes needed?

these changes are required for the private registry support we rolled out with image spec. if ngc_image_secret is specified, we still go the old route. if the user provides the image pull secret as a secret and adds builder options to the image spec, we don't need to specify the image pull secret in the pod template.

What changes were proposed in this pull request?

made ngc_image_secret optional.

How was this patch tested?

tested the NIM plugin by specifying the image pull secret as a flyte secret and adding it to imagespec.

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

Summary by Bito

This pull request enhances the NIM plugin by making the 'ngc_image_secret' parameter optional and introducing new builder options for improved private registry support, while maintaining compatibility with the old method. These changes aim to streamline the configuration process for users.

Signed-off-by: Samhita Alla <[email protected]>
@flyte-bot
Copy link
Contributor

Bito Automatic Review Skipped - Draft PR

Bito didn't auto-review because this pull request is in draft status.
No action is needed if you didn't intend for the agent to review it. Otherwise, to manually trigger a review, type /review in a comment and save.
You can change draft PR review settings here, or contact your Bito workspace admin at [email protected].

Signed-off-by: Samhita Alla <[email protected]>
@samhita-alla samhita-alla changed the title remove image pull secret make ngc image secret optional Jun 19, 2025
@samhita-alla samhita-alla marked this pull request as ready for review June 19, 2025 06:32
Signed-off-by: Samhita Alla <[email protected]>
@codecov
Copy link

codecov bot commented Jun 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.78%. Comparing base (3c6b61d) to head (69feb8f).
Report is 7 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (3c6b61d) and HEAD (69feb8f). Click for more details.

HEAD has 39 uploads less than BASE
Flag BASE (3c6b61d) HEAD (69feb8f)
41 2
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #3280       +/-   ##
===========================================
- Coverage   83.35%   52.78%   -30.57%     
===========================================
  Files         347      218      -129     
  Lines       28791    22715     -6076     
  Branches     2960     2961        +1     
===========================================
- Hits        23999    11991    -12008     
- Misses       3956    10006     +6050     
+ Partials      836      718      -118     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@samhita-alla
Copy link
Contributor Author

need a code owner review as well. @cosmicBboy would you mind reviewing the PR?

@cosmicBboy cosmicBboy merged commit 50911dc into master Jun 25, 2025
226 of 230 checks passed
Atharva1723 pushed a commit to Atharva1723/flytekit that referenced this pull request Oct 5, 2025
* remove image pull secret

Signed-off-by: Samhita Alla <[email protected]>

* make it optional

Signed-off-by: Samhita Alla <[email protected]>

* update docstring

Signed-off-by: Samhita Alla <[email protected]>

---------

Signed-off-by: Samhita Alla <[email protected]>
Signed-off-by: Atharva <[email protected]>
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.

5 participants