KEH-921 - Post-Migration Cleanup #57
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
TotalDwarf03
reviewed
Aug 5, 2025
Contributor
TotalDwarf03
left a comment
TotalDwarf03
left a comment
There was a problem hiding this comment.
Couple of minors. Good work 👍
Member
Author
|
All comments above should now be addressed |
TotalDwarf03
reviewed
Aug 6, 2025
Contributor
TotalDwarf03
left a comment
TotalDwarf03
left a comment
There was a problem hiding this comment.
Couple of comments still haven't been addressed.
- Docstrings + function type hints in
main.py - Backticks in
README.mdfor directory names - Need to remove
sdp-sandboxrefs
sebtheo
approved these changes
Aug 7, 2025
Contributor
sebtheo
left a comment
sebtheo
left a comment
There was a problem hiding this comment.
RIP dashboard. I haven't run locally but presume you have deployed to sdp-dev to test?
TotalDwarf03
approved these changes
Aug 7, 2025
Contributor
TotalDwarf03
left a comment
TotalDwarf03
left a comment
There was a problem hiding this comment.
Looks good to me. One minor comment to do but I'll approve in advance.
Good work on this 👍
src/main.py
Outdated
Contributor
There was a problem hiding this comment.
json isn't a valid type I don't think.
Believe this should a list[dict]
TotalDwarf03
added a commit
that referenced
this pull request
Aug 22, 2025
* Remove dashboard Terraform * Remove dashboard related files * Update Poetry and remove Streamlit * Remove pandas, matplotlib, plotly * Update and move CODEOWNERS to .github * Copy lambda README to root * Remove dashboard Dockerfile * Update README prerequisites and add makefile and table of contents sections * Remove dashboard dockerignore * Rename lambda folder to src * Add Makefile commands for applying linting * Run linters * Improve README formatting * Refactor lambda - Fix var typo - Capitalise constants - CoPilot -> Copilot - Add missing docstring - Remove unused var * Move terraform * Refactor lambda - Add module docstring - Use isinstance check - Use lazy formatting for logging - Remove unnecessary else and function parameter * Create LICENSE * Fail CI if linting fails * Potential fix for code scanning alert no. 5: Clear-text logging of sensitive information Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> * Remove old info from docs * Refactor lambda - Use Optional typing - Get Pylint to ignore handler parameters - Extract repeated logic for updating S3 - Extract logic for converting data to dictionary * Reduce linter max line length to 100 to fix Pylint conflict * Replace try except blocks with response type checks * Split handler into separate functions: reached 10/10 on pylint * Add tests folder * Update Makefile - Clean more temp files - Run linters on tests folder - Add new command for running tests * Update README - Add table of contents - Add testing and linting info * feat: update and setup for testing + coverage. - Added pytest-cov and pytest-xdist for enhanced testing capabilities. - Updated Makefile to include new test commands. - Modified .gitignore to exclude coverage reports. - Update README.md table of contents. * chore: update makefile commands - Remove tests directory from linting and formatting commands in Makefile. - Add .coverage to clean command in Makefile. * test: Write some tests for main.py - Test get_team_history - Test get_and_update_copilot_teams - Test update_s3_object - Add VS Code settings to gitignore * chore: remove old commented out tests * Add missing tests to reach 100% coverage * Add tests to ci.yml * Fix failing tests * Install dependencies via Poetry from requirements txt files and update all dependencies * Group Poetry docs dependencies * Use Poetry for mkdocs - Delete requirements txt file - Update deployment workflow - Update setup documentation * Fix Docker run command formatting * Delete requirements.txt * refactor: Update Dockerfile to use Poetry - Moved Dockerfile to root directory for better visibility. - Update Dockerfile to use Poetry for dependency management instead of pip. - Update .dockerignore to exclude unnecessary files from build context. - Update README.md to reflect move of Dockerfile to root directory. * Add option to run lambda outside of a container - Update README with setup instructions - Add commented function call to main.py * Address comments - Specify where to find CI workflow - Update sentence saying linting auto runs on tests - Fix README env vars - Use poetry install --only docs for mkdocs deployment - Add missing docstrings and return types - Return boolean for update_s3_object - Add comment above dev code * Run linting * Update README and .dockerignore - Remove disclaimer from README and update overview section with dashboard repository link - Add .github/ to .dockerignore * Remove and replace sandbox references - Remove and replace sandbox from .sh files, README, example_tfvars, variables.tf - Delete sandbox Terraform and move its example_tfvars to dev folder * Add backticks to README * Address comments - Use lambda full name - Update documentation bash - Move handler function to bottom of script * Address comments - Move logger to end of handler function - Remove Digital Landscape links - Specify Docker endpoint should return message if successful * Add missing type hints and return types * Tidy lambda scripts info in README * Fix function return type --------- Co-authored-by: Kieran Pritchard <kieran.pritchard@ons.gov.uk>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this? (check all applicable)
What
Cleaned up the repository following the Copilot dashboard migration to remove old files, dependencies, and Terraform.
Made several improvements e.g. refactoring lambda, adding tests, fixing linting checks, adding a license, getting linting and testing to run in CI.
Testing
Have any new tests been added as part of this issue? If not, try to explain why test coverage is not needed here.
Please write a brief description of why test coverage is not necessary here.
Documentation
Has any new documentation been written as part of this issue? We should try to keep documentation up to date
as new code is added, rather than leaving it for the future.
Please write a brief description of why documentation is not necessary here.
Related issues
KEH-921
How to review