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

[DEV-103519] Upgrade to Django 4.2. Switch from travis to GHA #66

Merged
merged 21 commits into from
Feb 23, 2024

Conversation

JVenberg
Copy link
Contributor

@JVenberg JVenberg commented Feb 22, 2024

What is the reason for this PR?

We are upgrading this library to support Django 4.2. As a result, we are updating the requirements to be Python>=3.8 and Django >=3.2

We no longer use travis to run our CI/CD tests. Our standard here at Rover is to use Github Actions. This switches the testing of different python versions over to GHA.

Validation

How will we know this is working?

  • This PR should have all tests passing.
  • After merging, the master branch should have all tests passing

sayhiben
sayhiben previously approved these changes Feb 22, 2024
Copy link
Contributor

@sayhiben sayhiben left a comment

Choose a reason for hiding this comment

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

Approved with the caveat that the Django 4 tests are failing due to what looks like a migration step that needs to be taken

.github/workflows/lint-and-test.yml Show resolved Hide resolved
JVenberg and others added 2 commits February 22, 2024 14:16
Co-authored-by: Ben Menesini <ben.menesini@rover.com>
@JVenberg JVenberg changed the title [DEV-103519] Switch from travis to GHA [DEV-103519] Upgrade to Django 4.2. Switch from travis to GHA Feb 22, 2024
@JVenberg JVenberg requested review from a team and sayhiben February 22, 2024 21:29
sayhiben
sayhiben previously approved these changes Feb 22, 2024
Copy link
Contributor

@sayhiben sayhiben left a comment

Choose a reason for hiding this comment

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

lgtm! Sorry if I led you down a rabbit hole with the cache; I didn't think it was going to be a PITA

.github/workflows/lint-and-test.yml Outdated Show resolved Hide resolved
setup.cfg Outdated

Choose a reason for hiding this comment

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

How did we settle on these options?

Copy link
Contributor Author

@JVenberg JVenberg Feb 22, 2024

Choose a reason for hiding this comment

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

I am not actually sure. I merged in @facundojmaero's PR here and he had changed a few things. I assume it was to either get it to lint with the newer linter version and/or due to the formatting changes he made

Choose a reason for hiding this comment

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

Oh gotcha. Yeah like the max line length change, for example. Not a huge deal since its just linting.

Choose a reason for hiding this comment

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

I would recommend you to migrate to ruff and deprecate all isort, flake8, pylint/black etc...

But it is up-to-you, just ruff is blazing fast and making what it should do without issues...

pearsondaniels
pearsondaniels previously approved these changes Feb 22, 2024
Copy link

@pearsondaniels pearsondaniels left a comment

Choose a reason for hiding this comment

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

Overall looks ok to me, just a question on some of the config. I don't have the Travis config for reference

Co-authored-by: Ben Menesini <ben.menesini@rover.com>
@JVenberg
Copy link
Contributor Author

Overall looks ok to me, just a question on some of the config. I don't have the Travis config for reference

Here is all we removed regarding Travis. I don't know if we've had a Github Travis integration for a long while

@JVenberg JVenberg merged commit d69f93b into master Feb 23, 2024
10 checks passed
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.

None yet

5 participants