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

contrib/net/http: add dynamic resource naming #1142

Merged
merged 17 commits into from Apr 28, 2022

Conversation

jcogilvie
Copy link
Contributor

Fixes #983.

@jcogilvie jcogilvie requested a review from a team January 24, 2022 15:34
@jcogilvie
Copy link
Contributor Author

This was built on v1.33 and I've been running it internally for a while. Looks like the main branch has moved ahead of me a bit and there's a conflict.

@gbbr gbbr changed the title Add dynamic resource naming contrib/net/http: Add dynamic resource naming Jan 26, 2022
@gbbr gbbr changed the title contrib/net/http: Add dynamic resource naming contrib/net/http: add dynamic resource naming Jan 26, 2022
contrib/net/http/option.go Outdated Show resolved Hide resolved
contrib/net/http/http.go Outdated Show resolved Hide resolved
@gbbr
Copy link
Contributor

gbbr commented Jan 26, 2022

Thanks for contributing this PR. The change is good, and will be accepted, but please reduce it only the what was discussed in the issue.

@ytakaya
Copy link

ytakaya commented Mar 2, 2022

@jcogilvie
Thanks for putting out the PR!
How is the progress of this?

@jcogilvie jcogilvie requested a review from a team as a code owner March 10, 2022 16:29
@jcogilvie
Copy link
Contributor Author

jcogilvie commented Mar 16, 2022

@gbbr The tests are passing on my code but failing on the tag, and I'm not sure what action I am expected to take. Otherwise I think I have addressed all of your feedback.

Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Looks good, one small nit.

contrib/net/http/http.go Outdated Show resolved Hide resolved
Accept suggestion.

Co-authored-by: Gabriel Aszalos <gabriel.aszalos@gmail.com>
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Nice! Thanks!

@gbbr gbbr added this to the 1.38.0 milestone Mar 21, 2022
@Julio-Guerra Julio-Guerra modified the milestones: 1.38.0, Triage Apr 15, 2022
@jcogilvie
Copy link
Contributor Author

@gbbr this was approved; can we get it merged?

@dianashevchenko
Copy link
Contributor

@jcogilvie please rebase to pass the checks and we'll merge 🙌

@jcogilvie
Copy link
Contributor Author

@dianashevchenko it's unclear to me how to make the tag test pass in test-core and test-core-appsec. Otherwise it passes.

@ajgajg1134
Copy link
Contributor

@jcogilvie, Those aren't your fault, the latest release hasn't been merged back into the v1 branch just yet which means the version number is mismatched on this PR. Once #1253 is merged you can rebase onto that change (there shouldn't be any conflicts) and everything will pass

@dianashevchenko
Copy link
Contributor

@jcogilvie the version number PR is merged, you can rebase now 👍

@jcogilvie
Copy link
Contributor Author

@dianashevchenko can we merge this yet? Seems like the goalposts keep moving.

@dianashevchenko dianashevchenko merged commit c7e6cbe into DataDog:v1 Apr 28, 2022
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.

proposal: “resource namer” for net/http wrapper
6 participants