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

ci: Test jaeger-v2 as all-in-one in CI #4890

Merged
merged 4 commits into from
Nov 3, 2023

Conversation

yurishkuro
Copy link
Member

@yurishkuro yurishkuro commented Oct 24, 2023

Which problem is this PR solving?

Description of the changes

  • Add cmd/jaeger-v2/Dockerfile
  • Parameterize scripts/build-all-in-one-image.sh to allow running jaeger-v2 instead of all-in-one
  • Call this script with jaeger-v2 from .github/workflows/ci-all-in-one-build.yml

How was this change tested?

$ BINARY=jaeger-v2 BRANCH=v2-docker GITHUB_SHA=abcd scripts/build-all-in-one-image.sh pr-only

@yurishkuro yurishkuro requested a review from a team as a code owner October 24, 2023 00:14
@yurishkuro yurishkuro added v2 changelog:ci Change related to continuous integration / testing labels Oct 24, 2023
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

see 2 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@yurishkuro yurishkuro changed the title ci: Test jaeger-v2 as all-in-one in CI [WIP] ci: Test jaeger-v2 as all-in-one in CI Oct 24, 2023
@yurishkuro
Copy link
Member Author

yurishkuro commented Oct 24, 2023

Marked this WIP because it fails to start jaeger-v2 due to what looks like the random ordering of extensions startup sequence, whereas jaegerquery extension can only run after jaegerstorage

open-telemetry/opentelemetry-collector#8732

@@ -51,6 +51,10 @@ jobs:
if: github.ref_name != 'main'
run: bash scripts/build-all-in-one-image.sh pr-only

- name: Build and test jaeger-v2 as all-in-one
if: github.ref_name != 'main'
run: BINARY=jaeger-v2 bash scripts/build-all-in-one-image.sh pr-only
Copy link
Member Author

Choose a reason for hiding this comment

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

this only added 39s to the total run, I think it's worth keeping in this workflow for now

@yurishkuro yurishkuro changed the title [WIP] ci: Test jaeger-v2 as all-in-one in CI ci: Test jaeger-v2 as all-in-one in CI Nov 2, 2023
albertteoh
albertteoh previously approved these changes Nov 2, 2023
@@ -35,35 +43,40 @@ run_integration_test() {
docker kill "$CID"
}

if [ "$mode" = "pr-only" ]; then
if [ -n "$pull_request" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest something like the following to help with readability:

is_pull_request=true

if [ "$is_pull_request" = true ]; then
if [ "$is_pull_request" = false ]; then

@@ -35,6 +38,11 @@ func newServer(config *Config, otel component.TelemetrySettings) *server {
}
}

// Dependencies implements extension.Dependent to ensure this always starts after jaegerstorage extension.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏼

Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
@@ -61,7 +61,7 @@ require (
go.opentelemetry.io/collector/exporter/loggingexporter v0.88.0
go.opentelemetry.io/collector/exporter/otlpexporter v0.88.0
go.opentelemetry.io/collector/exporter/otlphttpexporter v0.88.0
go.opentelemetry.io/collector/extension v0.88.0
go.opentelemetry.io/collector/extension v0.88.1-0.20231102040825-2e44da36e2c6
Copy link
Member Author

Choose a reason for hiding this comment

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

pinned to main branch for now, it will get updates once OTEL does a new release, which should happen before our next release in Dec

@yurishkuro yurishkuro merged commit 7ff9c3a into jaegertracing:main Nov 3, 2023
33 checks passed
@yurishkuro yurishkuro deleted the v2-docker branch November 3, 2023 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:ci Change related to continuous integration / testing v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants