-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[CI] Builder integration tests fail on release prep #6076
Comments
This is a tricky one. The problem is that in the release process, the If we simply hack in a replaces directive in the default builder config (for the purposes of integration tests), that won't work because the builder does its collector build in a temporary directory. Generating otelcorecol avoids this problem by passing the flags to generate code in the repository path and only doing the codegen. If we did the same in the builder tests, it would probably defeat the purpose of those tests. One thing that I found is that
|
How did this work for 0.59.0? |
#5752 was merged just two days ago, it was not on main when releasing 0.59.0 |
But we did have integration tests for the builder back then, and the problem would be similar. We just didn't have a default config file. |
The other configs are using replace or older versions |
I have to take another look, but I believe the integration tests should only use what we had before, while the new default config should be similar in test surface to what we had before. |
As part of #5752, I added a new integration test case to verify the default config. Since that PR was always rebased onto the latest main, the release tags were always present upstream and the test always passed. I don't know of a reasonable way to not run this test in the special case of the release process, but that could be an option if people prefer to do that. We could also simply drop that test case. I do sort of like the idea of the builder being able to always generate a collector using whatever the latest tag is, though the implementation is a bit hacky, and maybe it would eventually run into compatibility issues. |
I would replace that with regular unit tests, ensuring a config file can be generated. Whether a properly generated/crafted config file can be used to boot a collector is covered with the existing integration tests, which used to work during the releases (I believe).
The builder should generate a collector with the same version that was available when the builder was released (which is the same version number right now). The reason is that things might move on the collector side from a version to another, so we can only ensure the builder works for known versions of the collector. |
Drop the integration tests for the builtin builder configuration. We assume that it is a working config because it is generated from with the source tree and should be covered by collector tests. Add a unit test to ensure some basic sanity for the builtin configuration. This fixes open-telemetry#6076. Signed-off-by: James Peach <jpeach@cloudflare.com>
Drop the integration tests for the builtin builder configuration. We assume that it is a working config because it is generated from with the source tree and should be covered by collector tests. Add a unit test to ensure some basic sanity for the builtin configuration. This fixes open-telemetry#6076. Signed-off-by: James Peach <jpeach@cloudflare.com>
Drop the integration tests for the builtin builder configuration. We assume that it is a working config because it is generated from with the source tree and should be covered by collector tests. Add a unit test to ensure some basic sanity for the builtin configuration. This fixes #6076. Signed-off-by: James Peach <jpeach@cloudflare.com> Signed-off-by: James Peach <jpeach@cloudflare.com>
Describe the bug
On PRs that prepare the repo to release a new version, the integration tests for
ocb
fail, since the default configuration file references a tag that is not yet available on the repository.Steps to reproduce
Follow the release instructions. For example, see #6075
What did you expect to see?
All tests pass.
What did you see instead?
Integration tests for the builder fail with:
See full logs for more information.
Additional context
This happens since #5752 cc @jpeach
The text was updated successfully, but these errors were encountered: