add ldflags builder cmd option #6946
Conversation
Codecov ReportBase: 90.35% // Head: 90.35% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #6946 +/- ##
=======================================
Coverage 90.35% 90.35%
=======================================
Files 243 243
Lines 14567 14570 +3
=======================================
+ Hits 13162 13165 +3
Misses 1136 1136
Partials 269 269
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
kristinapathak
force-pushed
the
ldflags
branch
2 times, most recently
from
3f43f3e to
562c1ad
Compare
| SkipGetModules bool `mapstructure:"-"` | ||
| SkipCompilation bool `mapstructure:"-"` | ||
| SkipGetModules bool `mapstructure:"-"` | ||
| LDFlags string `mapstructure:"-"` |
There was a problem hiding this comment.
How do you expect this to be configured?
There was a problem hiding this comment.
As a command line argument. Ex:
builder --ldflags='-X "<pkg>.gitVersion=d49d45c039a2a89a26526afa3658de6429099bef" -X "<pkg>.gitTag=local-testing" -X "<pkg>.whoami=kristina.pathak"'
There was a problem hiding this comment.
where is the "--ldflags" flag defined?
There was a problem hiding this comment.
| SkipGetModules bool `mapstructure:"-"` | ||
| SkipCompilation bool `mapstructure:"-"` | ||
| SkipGetModules bool `mapstructure:"-"` | ||
| LDFlags string `mapstructure:"-"` |
There was a problem hiding this comment.
where is the "--ldflags" flag defined?
| SkipGetModules bool `mapstructure:"-"` | ||
| SkipCompilation bool `mapstructure:"-"` | ||
| SkipGetModules bool `mapstructure:"-"` | ||
| LDFlags string `mapstructure:"-"` |
There was a problem hiding this comment.
This should be part of "Distribution" and we should not have a flag but be part of the yaml config.
There was a problem hiding this comment.
Because there we have other flags, and because we want to go with "configuration as a code". Which means to pass the configuration to the builder as a config file merged into the code repository.
There was a problem hiding this comment.
In my example here, you can see that ldflags include git information. The moment these values are committed and pushed, they are incorrect since it will be a new commit hash with new git tags (our tags include datetimes). If ldflags become a configuration value, my automation would have to add it as part of the build process, and the value won't be able to be committed with the rest of the configuration file.
There was a problem hiding this comment.
@bogdandrutu, would you be open to moving this value under "Distribution" in the yaml config and allowing it to be set through a flag?
| assert.NoError(t, cfg.SetGoPath()) | ||
| require.NoError(t, GenerateAndCompile(cfg)) | ||
|
|
||
| // Sleep for 1 second to make sure all processes using the files are completed |
There was a problem hiding this comment.
This logic was taken from pre-existing main tests:
https://github.com/open-telemetry/opentelemetry-collector/blame/80cabdd33c50eaf386293f90fd8ac57cb3a10dc9/cmd/builder/internal/builder/main_test.go#L106
I see that now they have been refactored so will rebase and include my test case, but this logic still exists in those tests.
kristinapathak
force-pushed
the
ldflags
branch
from
d49d45c to
2b9ad71
Compare
|
|
|
I just triggered the failed test again. |
Description: Adding a feature -
ldflagsoption in builder cmd, to be passed togo buildLink to tracking Issue: #6940
Testing: manually built a collector with custom components who have build variables. Passing
ldflagsset the values of these variables.Documentation: I added a description for the
--helpcommand, but I can add a small explanation to the README if people would prefer.