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

[cmd/builder] add ocb feature to install go binary to temp dir #11386

Closed

Conversation

jackgopack4
Copy link
Contributor

Description

Removes requirement for ocb to have go binary installed in local environment. If go binary is not found in local path, downloads go binary for appropriate platform and architecture, based on the version that ocb was built with, installs to temporary directory, and deletes that directory when complete.

Link to tracking issue

Fixes #11382

Testing

Unit tests for each function added, as well as an additional test for SetGoPath() where $PATH is mocked to not have an existing go environment

Documentation

.chloggen file

Copy link

codecov bot commented Oct 7, 2024

Codecov Report

Attention: Patch coverage is 58.02469% with 34 lines in your changes missing coverage. Please review.

Project coverage is 91.78%. Comparing base (ef0c5be) to head (d4b47cc).
Report is 205 commits behind head on main.

Files with missing lines Patch % Lines
cmd/builder/internal/builder/config.go 59.49% 23 Missing and 9 partials ⚠️
cmd/builder/internal/builder/main.go 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11386      +/-   ##
==========================================
- Coverage   91.79%   91.78%   -0.02%     
==========================================
  Files         432      432              
  Lines       20425    20430       +5     
==========================================
+ Hits        18750    18752       +2     
+ Misses       1301     1294       -7     
- Partials      374      384      +10     

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

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added Stale and removed Stale labels Oct 24, 2024
@andrzej-stencel
Copy link
Member

This is an interesting solution to a make it easier for users to use the Builder without thinking about other dependencies - like to make sure Go is installed.

Still, I'm not convinced that downloading Go should be in scope of the Builder's responsibilities. This increases the complexity of the Builder's code. I'd rather keep the Builder's code as small as possible and have users use other tools to download Go. Happy to hear the maintainers' opinion on this.

@jackgopack4
Copy link
Contributor Author

@andrzej-stencel now that we have the docker images published I feel like that helps abstract away the requirement of having a local golang environment; there's of course also more security concerns with having ocb act as a "trojan horse," even if it only downloads golang to a temporary directory and removing afterwards. I don't feel strongly about this PR and I think we can probably let it expire in accordance with your comments about the issue #11382

@jpkrohling
Copy link
Member

I'm not convinced that downloading Go should be in scope of the Builder's responsibilities

I would vote to NOT have this feature. Setting up Go should not ocb's job.

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added Stale and removed Stale labels Nov 20, 2024
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.

remove local golang dependency for ocb binary
3 participants