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

[awsproxy] Expose service name as config option #29550

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

arpitjindal97
Copy link
Contributor

awsproxy extension can be used as proxy to any service not just xray

Description:
The service name variable was hardcoded to "xray". This PR exposes it to be configurable in the config.yaml

Link to tracking Issue:

Testing:

Documentation:
Appropriate README of the extension has been updated to reflect the addition of new option

CHANGELOG.md Outdated Show resolved Hide resolved
.chloggen/main.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

LGTM.

@arpitjindal97
Copy link
Contributor Author

@atoulme Can you approve the pipeline?

@arpitjindal97
Copy link
Contributor Author

@atoulme Can you approve the pipeline once again? some test were failing. I have fixed them.

@arpitjindal97
Copy link
Contributor Author

@atoulme All test passed. Can we merge it?

@atoulme
Copy link
Contributor

atoulme commented Nov 29, 2023

We need a review from codeowners. @dmitryax @Aneurysm9 please review

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Dec 27, 2023
@arpitjindal97

This comment was marked as outdated.

@arpitjindal97

This comment was marked as outdated.

@arpitjindal97
Copy link
Contributor Author

Hello team,

It has been 3 months since this PR is open. Seems like @Aneurysm9 is not active on this community anymore.

how do you plan to continue the development of this plugin moving forward?

cc: @atoulme @mxiamxia @djaglowski

Copy link
Member

@mxiamxia mxiamxia left a comment

Choose a reason for hiding this comment

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

LGTM

@bryan-aguilar
Copy link
Contributor

cd /home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/internal/tools && go build -o /home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/.tools/builder -trimpath go.opentelemetry.io/collector/cmd/builder
/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/.tools/builder --skip-compilation --config cmd/otelcontribcol/builder-config.yaml --output-path cmd/otelcontribcol
Flag --output-path has been deprecated, use config distribution::output_path
2024-02-27T20:54:39.623Z	INFO	internal/command.go:123	OpenTelemetry Collector Builder	{"version": "", "date": "unknown"}
2024-02-27T20:54:39.628Z	INFO	internal/command.go:159	Using config file	{"path": "cmd/otelcontribcol/builder-config.yaml"}
2024-02-27T20:54:39.628Z	INFO	builder/config.go:109	Using go	{"go-executable": "/opt/hostedtoolcache/go/1.21.7/x64/bin/go"}
2024-02-27T20:54:39.628Z	INFO	builder/main.go:67	You're building a distribution with non-aligned version of the builder. Compilation may fail due to API changes. Please upgrade your builder or API	{"builder-version": "0.94.0"}
2024-02-27T20:54:39.636Z	INFO	builder/main.go:91	Sources created	{"path": "cmd/otelcontribcol"}
2024-02-27T20:54:39.636Z	INFO	builder/main.go:25	Running go subcommand.	{"arguments": ["get", "cloud.google.com/go"]}
2024-02-27T20:54:41.684Z	INFO	builder/main.go:25	Running go subcommand.	{"arguments": ["mod", "tidy", "-compat=1.21"]}
2024-02-27T20:54:43.726Z	INFO	builder/main.go:142	Getting go modules
2024-02-27T20:54:43.726Z	INFO	builder/main.go:25	Running go subcommand.	{"arguments": ["mod", "download"]}
2024-02-27T20:54:44.331Z	INFO	builder/main.go:98	Generating source codes only, the distribution will not be compiled.
make --no-print-directory -C cmd/otelcontribcol fmt
Makefile:4: warning: overriding recipe for target 'lint'
../../Makefile.Common:199: warning: ignoring old recipe for target 'lint'
cd /home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/internal/tools && go build -o /home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/.tools/goimports -trimpath golang.org/x/tools/cmd/goimports
gofmt  -w -s ./
/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/.tools/goimports -w  -local github.com/open-telemetry/opentelemetry-collector-contrib ./
Generated code is out of date, please run "make genotelcontribcol" and commit the changes in this PR.

@arpitjindal97
Copy link
Contributor Author

Hello @bryan-aguilar ,

I have rebased my branch just now with latest main. Can you run the pipeline again?

@arpitjindal97
Copy link
Contributor Author

Seems like issues with podman permission, not related to my changes

# test install
echo
podman run --name $container_name -d $image_name
Error: OCI runtime error: chmod `run/shm`: Operation not supported
podman rm -fv otelcontribcol-deb-test >/dev/null 2>&1 || true

@bryan-aguilar
Copy link
Contributor

#31443

awsproxy extension can be used as proxy to any service not just xray

Signed-off-by: Arpit Agarwal <arpitjindal97@gmail.com>
@arpitjindal97
Copy link
Contributor Author

@bryan-aguilar I have rebased my branch again. Can you please run the CI again.

For some reason Github is showing 2 commits, but you can checkout locally it's just 1 commit.

@bryan-aguilar
Copy link
Contributor

bryan-aguilar commented Mar 1, 2024

@arpitjindal97 noted, I think force pushing every commit may be causing some issues. Maybe try merging and pushing without a force instead? Force pushing also overwrites commit history on the branch/PR which makes it harder for reviewers to see what changes have been made.

@bryan-aguilar bryan-aguilar added ready to merge Code review completed; ready to merge by maintainers and removed receiver/hostmetrics labels Mar 1, 2024
@arpitjindal97
Copy link
Contributor Author

@bryan-aguilar nevermind, github is showing correct commit now. It’s ready to merge ✌️

@codeboten codeboten merged commit ae8fde2 into open-telemetry:main Mar 13, 2024
150 of 151 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension/awsproxy internal/aws ready to merge Code review completed; ready to merge by maintainers receiver/awsxray
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants