-
Notifications
You must be signed in to change notification settings - Fork 0
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
Introduce Megalinter #40
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Ann, this was also to update megalinter to v8 which it does not look like has been done. Can you update to v8 and address any issues raised by linter as needed. There are references in the Makefile and GitHub actions yaml that will need updating to v8.
@AWitcherONS would you let me know when this is ready for another review? There are still some js and css files updated here that I don't think should be - but I don't know if there are more commits to come that you've not pushed yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of notes.
Best to link up with @helenb on the front-end linting bits
* Run CMS locally with supporting services in Docker (#38) * Added make commands * Added .env to .dockerignore * Extract dev backing services to their own compose * Added example .env file * Added runbook to the readme * Replaced comonly used commands to convenience aliases in the readme * Updated the makefile * Testing markdown rendering * Formating * Removed redundant settings in example .env * Readme fixes * Fixed typo in the README * Reverted the rename of the env_file in docker-compose.yml * Rename make commands, update the refrences in README and replace django-admin commands with make commands * Added the new section in the readme to the table of contents * Renamed the dev .env file to .development.env * Formatting + error fixes * Remove the 'expose' key from docker compose * Fixed typo Co-authored-by: Dan Braghiș <[email protected]> * Remove the dj aliases and changed the commands in README * Makefile fix * Remove the dj aliases and changed the commands in README (this time for real) * Added the make dev-init script and updated README * Attempt to format the Makefile to make CI pass * Added note about honcho picking up the mounted .env file in the README * Formating * Fixed the rendering of the warning block * Slight rewording * Added in the reword suggestion Co-authored-by: Jake Howard <[email protected]> * Replaced the warning block with an emoji * Updated outdated commands in the README * Move the prerequisites up in the README * Put the superuser auto-create in a separate make command and call it in dev-init command * Add the default superuser name and password in readme * Formating * Final comment fixes --------- Co-authored-by: Dan Braghiș <[email protected]> Co-authored-by: Jake Howard <[email protected]> * Add default heading 1 and breadcrumbs for templates that haven't yet had a banner added (#49) * Add default heading 1 and breadcrumbs for templates that haven't yet had a banner added * Don't show 2 h1s on the release calendar page * Support a mostly read-only database connection for external environment (#44) * Amend the runserver command in the Makefile (#52) * Don't show an full-screen overlay for warnings - only for errors (#51) * Move CODEOWNER file location (#53) * Use cupcake flavour * Fix lint issue + enable gitleaks --------- Co-authored-by: kacperpONS <[email protected]> Co-authored-by: Dan Braghiș <[email protected]> Co-authored-by: Jake Howard <[email protected]> Co-authored-by: Helen Chapman <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really close now. Left a few questions and suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. May be nice to address the tslint comments, could be a follow up if we can get it this week :D
LGTM - just tested it out! https://confluence.ons.gov.uk/display/NWP/Megalinter?flashId=428895879 will need to be updated as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all the files! LGTM!
What is the context of this PR?
Install and fix issues for megalinter
How to review
documentation https://confluence.ons.gov.uk/display/NWP/Megalinter?flashId=428895879
Follow-up Actions