Skip to content

Conversation

@blewis12
Copy link
Member

@blewis12 blewis12 commented Dec 11, 2025

PR Description

This is a follow up PR to to this previous iteration

This now introduces a new file type to the replacements YAML file for OCB manifests. I've done some small re-jigging to handle indentations a bit better as well as ensure that E2E tests work properly - these are all in separate commits for clarity.

Relates to #4719

PR Checklist

  • Tests updated

@blewis12 blewis12 requested a review from a team as a code owner December 11, 2025 14:49
Comment on lines 71 to +73
func upsertGeneratedBlock(targetContent, replacement, startMarker, endMarker string) (string, error) {
startIdx := strings.Index(targetContent, startMarker)
startFound := startIdx != -1
lineStart := strings.Index(targetContent, startMarker)
startFound := lineStart != -1
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it could be simpler if we:

  • Used Split to split the targetContent into slice of lines
  • Used Contains to find the line with start and end marker
  • Replaced the middle part of the slice
  • Used Join to join it all back into text

This way we don't need to have the complexity of lineStart etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

that's true 👍 let me give that a shot, sounds like it'd be more readable that way

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR extends the generate-module-dependencies tool to support OpenTelemetry Collector Builder (OCB) configuration files in addition to Go module files. The tool can now inject dependency replacement directives into OCB YAML config files using the same YAML-based configuration approach as Go modules, with appropriate comment markers and indentation for YAML syntax.

Key Changes:

  • Added ocb file type support alongside the existing mod file type
  • Implemented YAML-specific templating and comment markers (using # instead of //)
  • Refactored marker replacement logic to handle line-level operations (including leading/trailing content)
  • Separated E2E tests into file-type-specific test functions

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
tools/generate-module-dependencies/internal/types/types.go Adds FileTypeOCB constant and updates unmarshalling/error messages to support OCB file type
tools/generate-module-dependencies/internal/helpers/files.go Extends TemplatePath to return OCB template path for OCB file types
tools/generate-module-dependencies/internal/apply-replaces.go Adds OCB comment marker support (#) and refactors block replacement to operate on full lines including indentation
tools/generate-module-dependencies/replaces-ocb.tpl New template for OCB files with YAML-appropriate indentation and comment syntax
tools/generate-module-dependencies/cmd/sync-mod.go Refactors command initialization into a factory function for better testability
tools/generate-module-dependencies/e2e_test.go Splits E2E tests into separate TestE2EMod and TestE2EOCB functions for clarity
tools/generate-module-dependencies/testdata/basic-ocb/* Test fixtures for OCB files with empty generated section
tools/generate-module-dependencies/testdata/update-existing-ocb/* Test fixtures for OCB files with existing generated content to be updated
tools/generate-module-dependencies/README.md Documents OCB file type support with examples and configuration details

Comment on lines +86 to +91
// Find the start of the line containing the start marker
for lineStart > 0 && targetContent[lineStart-1] != '\n' {
lineStart--
}

searchFrom := lineStart + len(startMarker)
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

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

After moving lineStart backwards to the beginning of the line (lines 87-89), the calculation searchFrom := lineStart + len(startMarker) is incorrect. The original marker position needs to be preserved before moving lineStart backwards. The searchFrom position should be calculated from the original marker index (where the marker was found) plus the marker length, not from the beginning of the line. This will cause incorrect parsing of the end marker position.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about this, but maybe we can do the suggestion I've written about and this logic would go away 🤔

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