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.

Sign up for GitHub

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

Jump to bottom

Improve django action devex #13

Merged
merged 4 commits into from
Oct 1, 2023
Merged

Improve django action devex #13

merged 4 commits into from
Oct 1, 2023

Conversation

AaDalal
Copy link
Contributor

@AaDalal AaDalal commented Sep 18, 2023 •
edited
Loading

Rationale:

  1. it would be nice to see if tests pass before we go back and fix linting :)
  2. it would be nice to see why linting failed (since github actions env may differ from local results)

@AaDalal AaDalal requested a review from alxkp September 18, 2023 04:33
@AaDalal AaDalal changed the title Update action.yaml Improve django action devex Sep 19, 2023
joyliu-q
joyliu-q requested changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@joyliu-q joyliu-q left a comment

Choose a reason for hiding this comment

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

Have a few comments. Once this is merged, you will need to update your local action tag to reflect this new change.

django-check/action.yaml
pipenv run coverage combine
# container:
# image: python:${{ inputs.pythonVersion }}
container:
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason we want to bring this back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do all products use the same python version? If I remember correctly, that's not true, which is the rationale for bringing this back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm curious why we removed it, though

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add it back temporarily and monitor closely.

django-check/action.yaml Show resolved Hide resolved
django-check/action.yaml Show resolved Hide resolved
@AaDalal AaDalal requested review from joyliu-q and removed request for alxkp September 29, 2023 19:18
joyliu-q
joyliu-q approved these changes Oct 1, 2023
View reviewed changes
Copy link
Contributor

@joyliu-q joyliu-q left a comment

Choose a reason for hiding this comment

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

LGTM, let's monitor the container image temporarily

django-check/action.yaml
pipenv run coverage combine
# container:
# image: python:${{ inputs.pythonVersion }}
container:
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add it back temporarily and monitor closely.

@joyliu-q joyliu-q merged commit 3e595cc into main Oct 1, 2023
1 check passed
@joyliu-q joyliu-q deleted the reorder-backend-check branch October 1, 2023 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyliu-q joyliu-q joyliu-q approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AaDalal @joyliu-q