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

WIP [builder] Support for --skip-new-go-module proof of concept #9253

Closed
wants to merge 3 commits into from

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Jan 9, 2024

Description: Adds a --skip-new-go-module flag to the OTC builder. This enables users working in an existing go module environment (say, a "monorepo") to update the module they have, vs forcing the use of a new module.

With the new support inside an existing Go module, a collector main package can be generated using a go:generate directive. For example, in the directory where I want my collector built, the file generate.go has this line:

//go:generate builder --skip-new-go-module --skip-compilation --strict-versioning --config=./build-config.yaml

In the same directory, the build-config.yaml describes the collector to build. The builder generates the other files in the same directory. At this point, normal Go workflows can be used to update indirect dependencies. The optional --strict-versioning checks that the build configuration matches that resulting from the enclosing Go module.

Link to tracking Issue: #9252

Testing: WIP: No.

Documentation: Yes.

Copy link

codecov bot commented Jan 9, 2024

Codecov Report

Attention: 102 lines in your changes are missing coverage. Please review.

Comparison is base (6ed7ad1) 90.80% compared to head (04839d7) 90.34%.
Report is 8 commits behind head on main.

❗ Current head 04839d7 differs from pull request most recent head 156d318. Consider uploading reports for the commit 156d318 to get more accurate results

Files Patch % Lines
cmd/builder/internal/builder/main.go 11.95% 80 Missing and 1 partial ⚠️
service/internal/status/status.go 68.18% 6 Missing and 1 partial ⚠️
cmd/builder/internal/builder/config.go 60.00% 4 Missing and 2 partials ⚠️
...ce/internal/servicetelemetry/telemetry_settings.go 60.00% 3 Missing and 1 partial ⚠️
cmd/builder/internal/command.go 25.00% 2 Missing and 1 partial ⚠️
component/componenttest/nop_telemetry.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9253      +/-   ##
==========================================
- Coverage   90.80%   90.34%   -0.47%     
==========================================
  Files         341      341              
  Lines       18330    18446     +116     
==========================================
+ Hits        16645    16665      +20     
- Misses       1350     1441      +91     
- Partials      335      340       +5     

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

@jmacd
Copy link
Contributor Author

jmacd commented Jan 10, 2024

As discussed in today's Collector SIG call this is a proof that a solution to #9252 is a relatively simple change. I've added one more commit, implementing and documenting a "strict versioning" mode, which applies additional checks following the GetModules operation.

When strict versioning is configured,

  1. The existing INFO message about mismatch between the builder's own version and the configuration's otelcol_version field becomes an error.
  2. A mismatch between otelcol_version and the final go module becomes an error
  3. A mismatch between any component and the final go module becomes an error.

Each of these errors could have happened in esoteric and contrived cases without the new --skip-new-go-module functionality, for example if a component had a spurious dependency on the core library and the component in the build depends on a newer version of the core library. When --skip-new-go-module functionality is configured, these checks become a way for a user to know if the build configuration is up-to-date with the enclosing go module. (In a perfect world, we might want a command that automatically updates the build configuration instead of a strict failure mode, but having a strict failure mode is much less work than auto-updating a build configuration.)

At this point, this PR is exactly where I want it to be for my internal build workflow, which uses a monorepo and builds collectors using internal components. I surely hope this work will be accepted. I would like to hear that there is sponsorship for this idea before I add tests of all the new code. Approvers, please take a look, thanks -- if you express support, I'll add tests, thanks.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks @jmacd, I think this use-case makes sense to support. I'll be glad to review the PR once the tests are added 👍🏻

@jmacd
Copy link
Contributor Author

jmacd commented Jan 24, 2024

Have discussed @kristinapathak taking on this work, will close for now.

@jmacd jmacd closed this Jan 24, 2024
mx-psi added a commit that referenced this pull request Aug 22, 2024
A continuation of
#9253 and
#9631

Description: Adds a `--skip-new-go-module` flag to the OTC builder. This
enables users working in an existing go module environment (say, a
"monorepo") to update the module they have, vs forcing the use of a new
module.

With the new support inside an existing Go module, a collector main
package can be generated using a go:generate directive. For example, in
the directory where I want my collector built, the file generate.go has
this line:

//go:generate builder --skip-new-go-module --skip-compilation
--strict-versioning --config=./build-config.yaml
In the same directory, the build-config.yaml describes the collector to
build. The builder generates the other files in the same directory. At
this point, normal Go workflows can be used to update indirect
dependencies.

Link to tracking Issue:
#9252

Testing: Will add unit tests in the next few days.

Documentation: Yes.

---------

Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
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.

2 participants