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

Rewrite CI to a Modern Standard #21

Merged
merged 15 commits into from
May 17, 2024
Merged

Conversation

JasonGrace2282
Copy link
Member

@JasonGrace2282 JasonGrace2282 commented May 14, 2024

Motivation

Tin's CI is kind of weak, with a mix of black, autopep8 and isort that requires you to run the script before committing. Instead, we can use tools like pre-commit to auto-run these checks before committing, reducing the mental load on developers. Tin has a primitive form of pre-commit in place, but to utilize the concurrent nature of pre-commit it's better to use different hooks.
Also, the current script being run is very lax, more rules would result in better code quality with less mental load on developers.

TL;DR I hate running scripts.

Revamp CI

  • scripts/format.sh
  • scripts/check.sh
  • Be more strict with style

Linting

  • Clean up code

Bonus Request

  • Add pre-commit ci to auto push fixes, to automate this kind of thing.

@JasonGrace2282 JasonGrace2282 requested a review from a team as a code owner May 14, 2024 22:24
Copy link
Member

@krishnans2006 krishnans2006 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! A quick question:

.pre-commit-config.yaml Outdated Show resolved Hide resolved
@JasonGrace2282 JasonGrace2282 force-pushed the codespell branch 3 times, most recently from 16d8e43 to 1ceb7fc Compare May 15, 2024 11:28
@JasonGrace2282 JasonGrace2282 changed the title Improve pre-commit hooks Rewrite CI to a Modern Standard May 15, 2024
@JasonGrace2282 JasonGrace2282 force-pushed the codespell branch 3 times, most recently from e53a6b5 to fd1d003 Compare May 15, 2024 16:13
@JasonGrace2282
Copy link
Member Author

There seems to be a problem with setup-python, specifically actions/setup-python#436.

@JasonGrace2282 JasonGrace2282 force-pushed the codespell branch 2 times, most recently from a6b541c to 98f35d3 Compare May 15, 2024 22:12
Copy link
Member

@krishnans2006 krishnans2006 left a comment

Choose a reason for hiding this comment

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

Great work! Here are a few things I noticed while just reading over the changes. I'd like to test some of the more suspicious changes to make sure these won't break crucial functionality (even though they shouldn't), so this might not be the full review.

.pre-commit-config.yaml Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
tin/apps/assignments/forms.py Outdated Show resolved Hide resolved
tin/apps/assignments/views.py Outdated Show resolved Hide resolved
tin/apps/auth/oauth.py Show resolved Hide resolved
tin/apps/submissions/tasks.py Outdated Show resolved Hide resolved
tin/apps/users/models.py Outdated Show resolved Hide resolved
@JasonGrace2282
Copy link
Member Author

Thanks for the review!
Seeing as you added pre-commit.ci (thanks for that btw!) do you want me to just remove ci.yml, remove all the steps from it, or just leave it as it is (after removing the cache, because that failing is annoying)?

@krishnans2006
Copy link
Member

Seeing as you added pre-commit.ci (thanks for that btw!) do you want me to just remove ci.yml, remove all the steps from it, or just leave it as it is (after removing the cache, because that failing is annoying)?

You're welcome! It's quite sad that it fails for my hacky format.sh solution - luckily, this PR should fix it.

I think what you do with ci.yml is up to you. I don't think removing all its steps is a great option, but you can either delete it entirely or leave it as it is until we're sure that pre-commit.ci works perfectly.

@JasonGrace2282
Copy link
Member Author

I just went ahead and deleted it entirely :)
Better not to have to worry about pipenv cache causing the tests to fail imo.

@JasonGrace2282
Copy link
Member Author

On second thought, I just had an idea for a script making sure that Pipfile.lock is up-to-date. Maybe that would be a good replacement in the CI.

This is due to the removed instruction `pipenv install --dev --deploy`
which normally created the correct files
Copy link
Member

@krishnans2006 krishnans2006 left a comment

Choose a reason for hiding this comment

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

Great job! Are you good with merging this in, or are you planning more changes?

@JasonGrace2282
Copy link
Member Author

I think I'm good with this!

@krishnans2006 krishnans2006 merged commit 5134e8a into tjcsl:master May 17, 2024
2 checks passed
@JasonGrace2282 JasonGrace2282 deleted the codespell branch May 17, 2024 01:07
@JasonGrace2282 JasonGrace2282 mentioned this pull request May 30, 2024
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.

2 participants