Support mounting secrets by default builder. #3082

amitani · 2025-01-23T19:28:25Z

Tracking issue

Why are the changes needed?

Our dependencies tend to include libraries from our private GitHub repositories. The credential needs to be configured for the build process.

What changes were proposed in this pull request?

pip_secret_mounts is added to ImageSpec as an additional parameter. When this is set, the secret is added by the build command, and mounted at the specific RUN command to run pip/poetry/uv install.

This parameter is excluded when calculating ImageSpec.id, allowing to use a temporary path if needed. It is consistent with the behavior that the content of these files aren't taken into account for ImageSpec.id.

In the docstring, this parameter is explained as experimental. This allows more flexibility in the future, in case other mount type, e.g. to an environment variable, needs to be added.

How was this patch tested?

I tried locally with a poetry project that has private git dependency. Without this, the build (pyflyte build) fails installing these git dependencies. With this change, it succeeds.

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

Implements support for mounting secrets during image building in Flytekit by adding pip_secret_mounts parameter to ImageSpec. This feature allows specifying secret files and mount paths for Docker builds, particularly useful for handling private GitHub repository dependencies. The implementation includes validation checks for secret paths and comprehensive error handling, while ensuring these configurations don't affect ImageSpec ID calculation.

Unit tests added: True

Estimated effort to review (1-5, lower is better): 3

flyte-bot · 2025-01-23T19:28:40Z

Code Review Agent Run #74b08d

Actionable Suggestions - 1

flytekit/image_spec/default_builder.py - 1
- Check for secret availability · Line 476-477

Review Details

Files reviewed - 2 · Commit Range: e9913d8..d79a179
- flytekit/image_spec/default_builder.py
- flytekit/image_spec/image_spec.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

flyte-bot · 2025-01-23T19:32:48Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Feature Improvement - Secret Mount Support for Image Building	- `default_builder.py` - Added support for mounting secrets during pip/poetry/uv install commands - `image_spec.py` - Added pip_secret_mounts parameter to ImageSpec with validation logic - `test_default_builder.py` - Added tests for secret mounting functionality in DefaultImageBuilder - `test_image_spec.py` - Added validation tests for pip_secret_mounts parameter - `pydoclint-errors-baseline.txt` - Updated documentation error baseline to include new pip_secret_mounts parameter

flytekit/image_spec/default_builder.py

flyte-bot · 2025-01-23T20:35:29Z

Code Review Agent Run #951024

Actionable Suggestions - 0

Review Details

Files reviewed - 1 · Commit Range: d79a179..f055a8f
- flytekit/image_spec/default_builder.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

flyte-bot · 2025-01-23T22:45:29Z

Code Review Agent Run #53c8cb

Actionable Suggestions - 3

flytekit/image_spec/default_builder.py - 3
- Validate pip_github_credential_source · Line 475-476
- Potential hardcoded secret detected · Line 141-141
- Return type inconsistency in function · Line 201-201

Review Details

Files reviewed - 2 · Commit Range: bc26d5b..bc26d5b
- flytekit/image_spec/default_builder.py
- flytekit/image_spec/image_spec.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

flytekit/image_spec/default_builder.py

codecov · 2025-01-24T18:15:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.20%. Comparing base (f2db35a) to head (e2cf740).
Report is 1 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #3082       +/-   ##
===========================================
+ Coverage   46.98%   76.20%   +29.21%     
===========================================
  Files         202      202               
  Lines       21470    21493       +23     
  Branches     2767     2773        +6     
===========================================
+ Hits        10088    16379     +6291     
+ Misses      10886     4301     -6585     
- Partials      496      813      +317

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

flytekit/image_spec/image_spec.py

Signed-off-by: amitani <[email protected]>

flyte-bot · 2025-01-24T23:37:23Z

Code Review Agent Run #9436c8

Actionable Suggestions - 4

flytekit/image_spec/default_builder.py - 3
- Return type annotation mismatch with implementation · Line 188-188
- Consider using List over list type · Line 206-206
- Consider extracting GitHub credential URL construction · Line 224-230
flytekit/image_spec/image_spec.py - 1
- Consider adding GitHub credentials validation · Line 51-52

Additional Suggestions - 1

tests/flytekit/unit/core/image_spec/test_default_builder.py - 1
- Consider splitting ImageSpec initialization across lines · Line 334-334

Review Details

Files reviewed - 3 · Commit Range: 3ad2845..fd74fe4
- flytekit/image_spec/default_builder.py
- flytekit/image_spec/image_spec.py
- tests/flytekit/unit/core/image_spec/test_default_builder.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

flytekit/image_spec/default_builder.py

flytekit/image_spec/image_spec.py

flytekit/image_spec/default_builder.py

flytekit/image_spec/image_spec.py

Signed-off-by: amitani <[email protected]>

flyte-bot · 2025-02-01T01:29:17Z

Code Review Agent Run #47705e

Actionable Suggestions - 3

flytekit/image_spec/image_spec.py - 1
- Consider consolidating parameter validation logic · Line 134-143
tests/flytekit/unit/core/image_spec/test_image_spec.py - 2
- Test case tuple size mismatch · Line 166-166
- Test coverage could be more comprehensive · Line 293-296

Review Details

Files reviewed - 3 · Commit Range: 3c95a84..0e69ed3
- flytekit/image_spec/default_builder.py
- flytekit/image_spec/image_spec.py
- tests/flytekit/unit/core/image_spec/test_image_spec.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

flytekit/image_spec/image_spec.py

tests/flytekit/unit/core/image_spec/test_image_spec.py

flyte-bot · 2025-02-01T08:04:03Z

Code Review Agent Run #de6a86

Actionable Suggestions - 1

flytekit/image_spec/image_spec.py - 1
- Consider class constant for excluded parameters · Line 183-184

Review Details

Files reviewed - 3 · Commit Range: 0e69ed3..229cff2
- flytekit/image_spec/default_builder.py
- flytekit/image_spec/image_spec.py
- tests/flytekit/unit/core/image_spec/test_image_spec.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

flytekit/image_spec/image_spec.py

thomasjpfan

@amitani To fix the linting issue, can you try syncing with master so that it includes this fix:

#3090

Co-authored-by: Flyte Bot <[email protected]> Signed-off-by: amitani <[email protected]>

Signed-off-by: amitani <[email protected]>

flyte-bot · 2025-02-03T20:10:06Z

Code Review Agent Run #d15bb5

Actionable Suggestions - 2

flytekit/image_spec/image_spec.py - 1
- Consider adding path validation checks · Line 51-53
flytekit/image_spec/default_builder.py - 1
- Return type change may break callers · Line 192-192

Additional Suggestions - 1

flytekit/image_spec/image_spec.py - 1
- Consider more descriptive lambda parameter name · Line 163-163

Review Details

Files reviewed - 4 · Commit Range: 3ad2845..f9f49ba
- flytekit/image_spec/default_builder.py
- flytekit/image_spec/image_spec.py
- tests/flytekit/unit/core/image_spec/test_default_builder.py
- tests/flytekit/unit/core/image_spec/test_image_spec.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

Signed-off-by: amitani <[email protected]>

flytekit/image_spec/image_spec.py

flytekit/image_spec/default_builder.py

amitani · 2025-02-03T20:20:49Z

@amitani To fix the linting issue, can you try syncing with master so that it includes this fix:

#3090

Done. I ended up updating the baseline again to reflect the new change.

flyte-bot · 2025-02-03T21:20:07Z

Code Review Agent Run #075b08

Actionable Suggestions - 0

Review Details

Files reviewed - 1 · Commit Range: f9f49ba..e4e7a61
- pydoclint-errors-baseline.txt
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

amitani requested review from wild-endeavor, kumare3, eapolinario, pingsutw, cosmicBboy, samhita-alla, thomasjpfan and Future-Outlier as code owners January 23, 2025 19:28

amitani changed the title ~~GitHub token~~ Support using GitHub token by default builder. Jan 23, 2025

flyte-bot reviewed Jan 23, 2025

View reviewed changes

flytekit/image_spec/default_builder.py Outdated Show resolved Hide resolved

amitani commented Jan 23, 2025

View reviewed changes

flytekit/image_spec/default_builder.py Outdated Show resolved Hide resolved

amitani force-pushed the github_token branch from f055a8f to bc26d5b Compare January 23, 2025 21:44

flyte-bot reviewed Jan 23, 2025

View reviewed changes

flytekit/image_spec/default_builder.py Outdated Show resolved Hide resolved

flyte-bot reviewed Jan 23, 2025

View reviewed changes

flytekit/image_spec/default_builder.py Outdated Show resolved Hide resolved

wild-endeavor reviewed Jan 24, 2025

View reviewed changes

flytekit/image_spec/image_spec.py Outdated Show resolved Hide resolved

support github credential secret

3ad2845

Signed-off-by: amitani <[email protected]>

amitani force-pushed the github_token branch from bc26d5b to 3ad2845 Compare January 24, 2025 22:34

amitani added 3 commits January 24, 2025 14:37

Fix typehint

fe301d9

Signed-off-by: amitani <[email protected]>

Ruff format

1c2f77a

Signed-off-by: amitani <[email protected]>

Add test

fd74fe4

Signed-off-by: amitani <[email protected]>

flyte-bot reviewed Jan 24, 2025

View reviewed changes

flytekit/image_spec/default_builder.py Outdated Show resolved Hide resolved

flyte-bot reviewed Jan 24, 2025

View reviewed changes

flytekit/image_spec/default_builder.py Outdated Show resolved Hide resolved

flyte-bot reviewed Jan 24, 2025

View reviewed changes

flytekit/image_spec/default_builder.py Outdated Show resolved Hide resolved

flyte-bot reviewed Jan 24, 2025

View reviewed changes

flytekit/image_spec/image_spec.py Outdated Show resolved Hide resolved

flyte-bot reviewed Jan 31, 2025

View reviewed changes

flytekit/image_spec/default_builder.py Outdated Show resolved Hide resolved

flyte-bot reviewed Jan 31, 2025

View reviewed changes

flytekit/image_spec/default_builder.py Show resolved Hide resolved

flyte-bot reviewed Jan 31, 2025

View reviewed changes

flytekit/image_spec/default_builder.py Show resolved Hide resolved

flyte-bot reviewed Jan 31, 2025

View reviewed changes

flytekit/image_spec/default_builder.py Show resolved Hide resolved

flyte-bot reviewed Jan 31, 2025

View reviewed changes

flytekit/image_spec/default_builder.py Show resolved Hide resolved

flyte-bot reviewed Jan 31, 2025

View reviewed changes

flytekit/image_spec/default_builder.py Show resolved Hide resolved

flyte-bot reviewed Jan 31, 2025

View reviewed changes

flytekit/image_spec/image_spec.py Show resolved Hide resolved

Exclude secret mounts from id

abbe41f

Signed-off-by: amitani <[email protected]>

amitani requested review from thomasjpfan and wild-endeavor February 1, 2025 00:56

flyte-bot reviewed Feb 1, 2025

View reviewed changes

flytekit/image_spec/image_spec.py Show resolved Hide resolved

flyte-bot reviewed Feb 1, 2025

View reviewed changes

tests/flytekit/unit/core/image_spec/test_image_spec.py Show resolved Hide resolved

flyte-bot reviewed Feb 1, 2025

View reviewed changes

tests/flytekit/unit/core/image_spec/test_image_spec.py Outdated Show resolved Hide resolved

flyte-bot reviewed Feb 1, 2025

View reviewed changes

flytekit/image_spec/image_spec.py Show resolved Hide resolved

thomasjpfan reviewed Feb 3, 2025

View reviewed changes

amitani and others added 4 commits February 3, 2025 11:12

Update flytekit/image_spec/default_builder.py

eab741f

Co-authored-by: Flyte Bot <[email protected]> Signed-off-by: amitani <[email protected]>

Address flyte-bot comment.

660f013

Signed-off-by: amitani <[email protected]>

exclude pip_secret_mounts from tag too.

9375575

Signed-off-by: amitani <[email protected]>

Merge remote-tracking branch 'origin/master' into github_token

f9f49ba

Signed-off-by: amitani <[email protected]>

amitani force-pushed the github_token branch from e2cf740 to f9f49ba Compare February 3, 2025 19:13

amitani changed the title ~~Support using GitHub token by default builder.~~ Support mounting secrets by default builder. Feb 3, 2025

update pydoclint baseline

e4e7a61

Signed-off-by: amitani <[email protected]>

flyte-bot reviewed Feb 3, 2025

View reviewed changes

flytekit/image_spec/image_spec.py Show resolved Hide resolved

flyte-bot reviewed Feb 3, 2025

View reviewed changes

flytekit/image_spec/default_builder.py Show resolved Hide resolved

amitani requested a review from thomasjpfan February 3, 2025 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support mounting secrets by default builder. #3082

Support mounting secrets by default builder. #3082

amitani commented Jan 23, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Jan 23, 2025 •

edited

Loading

Code Review Agent Run #74b08d

flyte-bot commented Jan 23, 2025 •

edited

Loading

Changelist by Bito

flyte-bot commented Jan 23, 2025 •

edited

Loading

Code Review Agent Run #951024

flyte-bot commented Jan 23, 2025 •

edited

Loading

Code Review Agent Run #53c8cb

codecov bot commented Jan 24, 2025 •

edited

Loading

flyte-bot commented Jan 24, 2025 •

edited

Loading

Code Review Agent Run #9436c8

flyte-bot commented Feb 1, 2025 •

edited

Loading

Code Review Agent Run #47705e

flyte-bot commented Feb 1, 2025 •

edited

Loading

Code Review Agent Run #de6a86

thomasjpfan left a comment

flyte-bot commented Feb 3, 2025 •

edited

Loading

Code Review Agent Run #d15bb5

amitani commented Feb 3, 2025

flyte-bot commented Feb 3, 2025 •

edited

Loading

Code Review Agent Run #075b08

Support mounting secrets by default builder. #3082

Are you sure you want to change the base?

Support mounting secrets by default builder. #3082

Conversation

amitani commented Jan 23, 2025 • edited by flyte-bot Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

Related PRs

Docs link

Summary by Bito

flyte-bot commented Jan 23, 2025 • edited Loading

Code Review Agent Run #74b08d

flyte-bot commented Jan 23, 2025 • edited Loading

Changelist by Bito

flyte-bot commented Jan 23, 2025 • edited Loading

Code Review Agent Run #951024

flyte-bot commented Jan 23, 2025 • edited Loading

Code Review Agent Run #53c8cb

codecov bot commented Jan 24, 2025 • edited Loading

Codecov Report

flyte-bot commented Jan 24, 2025 • edited Loading

Code Review Agent Run #9436c8

flyte-bot commented Feb 1, 2025 • edited Loading

Code Review Agent Run #47705e

flyte-bot commented Feb 1, 2025 • edited Loading

Code Review Agent Run #de6a86

thomasjpfan left a comment

Choose a reason for hiding this comment

flyte-bot commented Feb 3, 2025 • edited Loading

Code Review Agent Run #d15bb5

amitani commented Feb 3, 2025

flyte-bot commented Feb 3, 2025 • edited Loading

Code Review Agent Run #075b08

amitani commented Jan 23, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Jan 23, 2025 •

edited

Loading

flyte-bot commented Jan 23, 2025 •

edited

Loading

flyte-bot commented Jan 23, 2025 •

edited

Loading

flyte-bot commented Jan 23, 2025 •

edited

Loading

codecov bot commented Jan 24, 2025 •

edited

Loading

flyte-bot commented Jan 24, 2025 •

edited

Loading

flyte-bot commented Feb 1, 2025 •

edited

Loading

flyte-bot commented Feb 1, 2025 •

edited

Loading

flyte-bot commented Feb 3, 2025 •

edited

Loading

flyte-bot commented Feb 3, 2025 •

edited

Loading