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

Assorted dev experience & CI fixes #138

Merged
merged 7 commits into from
Nov 5, 2024
Merged

Assorted dev experience & CI fixes #138

merged 7 commits into from
Nov 5, 2024

Conversation

Restioson
Copy link
Collaborator

Fixes include:

  • Running tests for specific modules from make test (pass module=...)
  • Added some notes to developers in the README
  • Designated all Makefile targets as PHONY
  • Added lint and fmt aliases to Makefile
  • Fix import order in test_frontend.py
  • Actually run the linter in CI

This is done by a variable that can be passed on the command line, e.g.
`make test module=general.tests.test_document_admin`
Describe some common commands and patterns to ease development of LwimiLinks
This prevents them from breaking if we ever accidentally create files with their
 names in the root directory of the project (and is also just good practice).
 See https://www.gnu.org/software/make/manual/html_node/Special-Targets.html for
  more
These are very commonly used command verbs, so they are nice to as shortcuts,
especially for people getting used to the project
We were not actually running the linter in CI (despite the name of the
job saying so)
@Restioson
Copy link
Collaborator Author

Restioson commented Nov 5, 2024

I've verified that the CI does now properly fail for both linting issues and formatting issues (added some commits here and then force-pushed to remove)

This was a linting failure from
[zizmor](https://blog.yossarian.net/2024/10/27/Now-you-can-have-beautiful-clean-workflows).
See actions/checkout#485 for more info on why this is
a potential security issue.
@Restioson
Copy link
Collaborator Author

The last commit probably doesn't affect us, but better to have it there in case we do one day add a sensitive GitHub token for use in login. I'm not sure if it may have an impact on the deployment workflow (this is the only one I am not sure how to test)

@@ -125,6 +130,7 @@ jobs:
- uses: actions/checkout@v4
with:
fetch-depth: 0
persist-credentials: false
Copy link
Contributor

Choose a reason for hiding this comment

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

To help in my understanding: how does a later git fetch work if the credentials aren't there any more?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it works because we only need public auth for the following fetch

@Restioson Restioson merged commit 61c3f07 into main Nov 5, 2024
3 checks passed
@Restioson Restioson deleted the chore/misc-dx-fixes branch November 5, 2024 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants