Skip to content

[builder] Rewrite replace paths to be absolute in generated go.mod #12638

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

milas
Copy link

@milas milas commented Mar 15, 2025

Description

Convert all relative paths to be absolute for replace directives the same way that is done for modules/components.

This ensures that the output_path isn't tied to the location of the builder YAML config.

For example:

/
├── work
│   ├── mycollector
│   │   └── builder.yaml
│   ├── mylib
│   │   └── go.mod
└── dist
    └── my-collector

OCB YAML - /work/mycollector/builder.yaml

replaces:
 - registry.test/somemodule => ../mylib

Build

cd /work/mycollector
DIST_OUTPUT_PATH=/dist ocb --config ./builder.yaml

Before these changes

  • Generated go.mod's replace would end up resolving to /mylib

After these changes

  • Generated go.mod's replace resolves to /work/mylib

Link to tracking issue

n/a

Testing

Added unit test

Documentation

Copious comments in code, nothing user-facing

@milas milas requested a review from a team as a code owner March 15, 2025 05:43
@milas milas requested a review from atoulme March 15, 2025 05:43
@milas milas changed the title feat(ocb): support arbitrary replace paths [builder] rewrite replace paths to be absolute in generated go.mod Mar 15, 2025
@milas milas changed the title [builder] rewrite replace paths to be absolute in generated go.mod [builder] Rewrite replace paths to be absolute in generated go.mod Mar 15, 2025
@milas milas force-pushed the milas/ocb-replace-abspath branch from 10e30a0 to f41cd8f Compare March 15, 2025 13:33
Convert all relative paths to be absolute for `replace` directives
the same way that is done for modules.

This ensures that the `output_path` isn't tied to the location of
the builder YAML config.
@milas milas force-pushed the milas/ocb-replace-abspath branch from f41cd8f to 5875a50 Compare March 15, 2025 15:54
Copy link

codecov bot commented Mar 15, 2025

Codecov Report

Attention: Patch coverage is 58.53659% with 17 lines in your changes missing coverage. Please review.

Project coverage is 91.46%. Comparing base (eb00c82) to head (b75508d).
Report is 127 commits behind head on main.

Files with missing lines Patch % Lines
cmd/builder/internal/builder/config.go 58.53% 12 Missing and 5 partials ⚠️

❌ Your patch check has failed because the patch coverage (58.53%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12638      +/-   ##
==========================================
- Coverage   91.50%   91.46%   -0.05%     
==========================================
  Files         480      480              
  Lines       26464    26499      +35     
==========================================
+ Hits        24217    24238      +21     
- Misses       1778     1788      +10     
- Partials      469      473       +4     

☔ 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.

@milas milas force-pushed the milas/ocb-replace-abspath branch from 5c8074c to b75508d Compare March 26, 2025 03:43
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 the Stale label Apr 10, 2025
@milas
Copy link
Author

milas commented Apr 11, 2025

RE: stale label - Happy to make any changes / rebase on main based on review if of interest to maintainers. Note that I'm not sure what's up with codecov, as there are unit test cases included. Otherwise, will let stalebot close.

@atoulme
Copy link
Contributor

atoulme commented Apr 11, 2025

It's unclear if we would like to adopt this change. Please take a look at the checks CI failure - this is generating a path that is absolute and therefore no longer portable across machines. When would this approach make sense?

You also need to add a changelog entry for this PR. make chlog-new to generate the yaml template.

@github-actions github-actions bot removed the Stale label Apr 11, 2025
@milas
Copy link
Author

milas commented Apr 11, 2025

Thanks for the reply!

this is generating a path that is absolute and therefore no longer portable across machines

Correct. I agree that behavior is not ideal, but fwiw it's the status quo:

// Check if path is empty, otherwise filepath.Abs replaces it with current path ".".
if mod.Path != "" {
var err error
mod.Path, err = filepath.Abs(mod.Path)

Today, if you do:

providers:
  - gomod: example.com/module v1.0.0
    path: ./mymodule

The resulting go.mod produced by ocb will have an absolute/non-portable path in the generated replace directive.

However, if you do:

replaces:
  - example.com/other => ./myvendor

The resulting go.mod produced by ocb will NOT have an absolute path for that replace directive.

In practice, this will break things:

/home/milas/mycollector/
  mymodule/
    go.mod
  myvendor/
    go.mod
  builder.yaml

When you run ocb, you'll end up with /home/milas/mycollector/dist/go.mod:

replace example.com/module v1.0.0 => /home/milas/mycollector/mymodule
replace example.com/other => ./myvendor

However, the correct relative path would be ../myvendor here, so it does not work as expected currently IMO, even non-portably.

Arguably, OCB could instead try to generate relative paths for the "dist" path based on the builder.yaml path: I did not attempt that for this since the module/component replacement was already using absolute/non-portable paths for the local replacement use case.

As a workaround, if you know the dist path will be a child, you can manually make the paths ../ instead of ./, though this inconsistent with the behavior of other paths.

In particular, that breaks my CI build flow (in a Docker container):

  • Run DIST_OUTPUT_PATH=/dist ocb --config builder.yaml /src/builder-config.yaml --skip-compilation (scaffold but don't build)
  • From /dist, run go build

Currently, I end up with a go.mod where my components have replace ... => /src/mymodule (works) but the generic "replace" has replace ... => ./myvendor (does not work).


You also need to add a changelog entry for this PR. make chlog-new to generate the yaml template.

ACK re: this + unit tests, will hold off for the moment until it's agreed that this makes sense in its current form -- I realize it's a pretty niche use case 😉

Copy link
Contributor

github-actions bot commented May 1, 2025

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

@github-actions github-actions bot added the Stale label May 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants