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

added fastapi.yml and updated django and flask #2363

Merged
merged 28 commits into from
Apr 30, 2021
Merged

added fastapi.yml and updated django and flask #2363

merged 28 commits into from
Apr 30, 2021

Conversation

Genegrady
Copy link
Contributor

Description

Checklist

  • Added to the correct milestone.
  • Tests provided or description of manual testing performed is included in the code or PR.
  • Library documentation is updated.
  • Corp site documentation is updated (link to the PR).

@Genegrady Genegrady requested a review from a team as a code owner April 27, 2021 19:27
.github/workflows/fastapi.yml Outdated Show resolved Hide resolved
.github/workflows/fastapi.yml Outdated Show resolved Hide resolved
@Genegrady Genegrady closed this Apr 27, 2021
@Genegrady Genegrady reopened this Apr 27, 2021
@Genegrady Genegrady added the changelog/no-changelog A changelog entry is not required for this PR. label Apr 28, 2021
working-directory: fastapi
strategy:
matrix:
python-version: [3.6, 3.7, 3.8, 3.9]
Copy link
Member

Choose a reason for hiding this comment

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

think I'll echo a previous comment, we can probably just test 1 version of Python, since FastAPI is testing that they are compatible with all of these versions, we just need to be sure we are compatible with FastAPI

(unless our instrumentation has a bunch of python version conditionals?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brettlangdon Sounds good. I will change the conditional to the latest version (3.9)

Copy link
Contributor

@Yun-Kim Yun-Kim 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 to me once some comments are added to the workflow. Just curious, what does the output of the tests look like when you run this action locally?

Comment on lines 25 to 30
- name: Install Flit
run: pip install flit
- name: Install Dependencies
run: flit install --symlink
- name: Downgrade SQLalchemy
run: pip install --force-reinstall SQLAlchemy==1.3.24
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if you could add a quick comment above each of these these steps to label as being fastapi specific, and maybe explain why these steps are necessary to provide some context in the future 😄

Copy link
Contributor Author

@Genegrady Genegrady left a comment

Choose a reason for hiding this comment

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

Added comments to FastAPI specific steps.

Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

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

👏 lgtm

@brettlangdon brettlangdon merged commit ab1a7a7 into master Apr 30, 2021
@brettlangdon brettlangdon deleted the fastapi branch April 30, 2021 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants