Skip to content

Fix fastapi version#1664

Merged
srikanthccv merged 2 commits intoopen-telemetry:mainfrom
ocelotl:issue_1663
Feb 13, 2023
Merged

Fix fastapi version#1664
srikanthccv merged 2 commits intoopen-telemetry:mainfrom
ocelotl:issue_1663

Conversation

@ocelotl
Copy link
Copy Markdown
Contributor

@ocelotl ocelotl commented Feb 12, 2023

Fixes #1663

@ocelotl ocelotl added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Feb 12, 2023
@ocelotl ocelotl self-assigned this Feb 12, 2023
@ocelotl ocelotl requested a review from a team February 12, 2023 20:13
@shalevr
Copy link
Copy Markdown
Member

shalevr commented Feb 12, 2023

opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/package.py

Line 16 in 3bcc9fb

_instruments = ("fastapi ~= 0.58",)

Does this need to be updated too?

srikanthccv
srikanthccv reviewed Feb 12, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

I am not sure about fixing the version. Looking at the CI failures, it's just the test_uninstrument_app failing, and after reading the CHANGELOG https://github.com/tiangolo/fastapi/releases/tag/0.91.0. Can we update the failing test instead?

@ocelotl
Copy link
Copy Markdown
Contributor Author

ocelotl commented Feb 13, 2023

I am not sure about fixing the version. Looking at the CI failures, it's just the test_uninstrument_app failing, and after reading the CHANGELOG https://github.com/tiangolo/fastapi/releases/tag/0.91.0. Can we update the failing test instead?

Sure, but better to do that in other PR, right now this is stopping a regression fix in #1660 from entering into main.

@ocelotl
Copy link
Copy Markdown
Contributor Author

ocelotl commented Feb 13, 2023

opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/package.py

Line 16 in 3bcc9fb

_instruments = ("fastapi ~= 0.58",)

Does this need to be updated too?

Right, fixed ✌️

@srikanthccv srikanthccv mentioned this pull request Feb 13, 2023
Closed
@srikanthccv
Copy link
Copy Markdown
Member

srikanthccv commented Feb 13, 2023

Created #1665 to fix it separately.

@srikanthccv srikanthccv merged commit bb41b46 into open-telemetry:main Feb 13, 2023
@phillipuniverse phillipuniverse mentioned this pull request Mar 12, 2023
Merged
10 tasks
@phillipuniverse phillipuniverse mentioned this pull request Apr 7, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shalevr shalevr shalevr approved these changes

+1 more reviewer

@srikanthccv srikanthccv srikanthccv left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@ocelotl ocelotl

Labels

Skip Changelog PRs that do not require a CHANGELOG.md entry

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Fix fastapi version

3 participants

@ocelotl @shalevr @srikanthccv