Pre commit additions #3

AoifeHughes · 2024-04-30T14:43:44Z

Moving this PR to here

AoifeHughes · 2024-04-30T14:44:42Z

Currently setting up some GitHub workflows. Im assuming there's no need for it, but making a note here that it would be straight-forward to add something like

    needs: pre-commit
    runs-on: ubuntu-latest

    steps:
    - uses: actions/checkout@v3
      
    - name: Build Docker image
      run: |
        docker build -t your-docker-image:${{ github.sha }} .
      
    - name: Log in to Docker Hub
      uses: docker/login-action@v2
      with:
        username: ${{ secrets.DOCKERHUB_USERNAME }}
        password: ${{ secrets.DOCKERHUB_PASSWORD }}
      
    - name: Push Docker image
      run: |
        docker push your-docker-image:${{ github.sha }}

to auto reply this to docker hub - if we ever wanted to move to azure or similar then this would make deployment even easier. Though I don't understand enough about security concerns behind this repo

AoifeHughes · 2024-04-30T14:44:53Z

todo:

AoifeHughes · 2024-05-19T21:46:48Z

Updating to say coverage.py isn't really needed, now are the R features given how this repo is setup.

AoifeHughes · 2024-05-19T21:54:49Z

@HChughtai I just pushed some documentation and docker file changes. I think given what we discussed last week and the complications about making usage assumptions (pre-installing pre-commit to clear jupyter cells etc) that this is some reasonable changes that could be folded back to main.

HChughtai

Hi @AoifeHughes - the addition of pre-commit and changes to the Dockerfile broadly look good to me. I've made some suggestions on updating dependencies and some specific pre-commit hooks. Some of these may be more personal preferences, so feel free to ignore!

There also looks to be a merge conflict to resolve, and I'd recommend ensuring running pre-commit passess locally on all files in the branch too before merging (I got some alerts about secrets detected in users/sample-user.env so they may need to be ignored)

(I also noticed that when I try to run some of the scripts that the execution bit isn't set, so that might also be a good addition to the pre-commit checks)

HChughtai · 2024-05-22T08:54:21Z

.github/workflows/ci.yml

+  pre-commit:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v3


Suggested change

- uses: actions/checkout@v3

- uses: actions/checkout@v4

HChughtai · 2024-05-22T08:55:01Z

.github/workflows/ci.yml

+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v3
+    - uses: actions/setup-python@v4


Suggested change

- uses: actions/setup-python@v4

- uses: actions/setup-python@v5

HChughtai · 2024-05-22T08:58:58Z

.github/workflows/ci.yml

+        pip install pre-commit
+    - name: Run pre-commit
+      run: |
+        pre-commit run --all-files


Suggested change

pre-commit run --all-files

pre-commit run --all-files --color always --verbose

Suggesting this so that debugging is a bit easier. You might also want to consider caching pre-commit using https://github.com/marketplace/actions/cache to speed things up.

HChughtai · 2024-05-22T09:02:16Z

.pre-commit-config.yaml

I'd suggest updating the versions of the repos here to the latest. It might also be worth considering setting up Dependabot or another automated dependency update tool to help, but I think that's out of scope of this PR and could be considered seperatly

HChughtai · 2024-05-22T09:04:43Z

.pre-commit-config.yaml

+  - repo: https://github.com/psf/black
+    rev: 22.8.0
+    hooks:
+      - id: black
+        name: black - consistent Python code formatting (auto-fixes)
+        language_version: python
+        exclude: test_example_module.py|run_pipeline.py
+
+  - repo: https://github.com/PyCQA/flake8
+    rev: 5.0.4
+    hooks:
+      - id: flake8
+        name: flake8 - Python linting
+        exclude: test_example_module.py|run_pipeline.py


You might want to consider using https://github.com/astral-sh/ruff-pre-commit instead as it's gaining popularity in the Python community and is faster

HChughtai · 2024-05-22T09:05:52Z

.pre-commit-config.yaml

+  - repo: https://github.com/nbQA-dev/nbQA
+    rev: 1.5.3
+    hooks:
+      - id: nbqa-black
+        name: nbqa-black - Format Jupyter Notebooks with black
+        additional_dependencies: [ black==22.8.0 ]
+      - id: nbqa-isort
+        name: nbqa-isort - Sort imports in Jupyter Notebooks with isort
+        additional_dependencies: [ isort==5.12.0 ]


https://github.com/astral-sh/ruff-pre-commit can also run on Jupyter notebooks too but I haven't used it there so I can't comment on how it performs vs nbQA

HChughtai · 2024-05-22T09:20:51Z

README.md

-This project is based on Jon Ghillam's repository. https://github.com/space-safety/jupyter/
+This project is based on Jon Ghillam's repository: [space-safety/jupyter](https://github.com/space-safety/jupyter/).
+
+## Getting Started


It might be worth including a bit on how the different scripts under scripts are used to create the image, and document that the configs under users are Docker Compose yaml as it could help people new to the repo figure things out - again, just a suggestion and could be out of scope here.

HChughtai · 2024-05-22T09:23:59Z

Dockerfile

-RUN wget -q https://github.com/quarto-dev/quarto-cli/releases/download/v1.4.543/quarto-1.4.543-linux-amd64.deb && \
+    rm -f ./google-chrome-stable_current_amd64.deb && \
+    wget -q https://github.com/quarto-dev/quarto-cli/releases/download/v1.4.543/quarto-1.4.543-linux-amd64.deb && \
    apt-get install --yes --no-install-recommends ./quarto-1.4.543-linux-amd64.deb && \
-    rm -f ./quarto-1.4.543-linux-amd64.deb


Thinking out loud, and again maybe out of scope for this PR - would it be useful to set the version number of Quarto elsewhere as a variable so that it can be easily updated in a single place?

HChughtai · 2024-05-22T09:25:41Z

Dockerfile

It's outside your PR, but I'm also wondering if the base image here needs to be updated?

AoifeHughes added 3 commits April 29, 2024 15:53

Added pre-commit hooks for code formatting and linting

8eaf1b4

init precommit run

9b0d6ec

Add CI workflow for pre-commit checks

6907956

AoifeHughes self-assigned this Apr 30, 2024

AoifeHughes added the documentation Improvements or additions to documentation label Apr 30, 2024

HChughtai self-requested a review May 16, 2024 13:04

chore: Update JupyterLab Docker services README

bf9060b

HChughtai requested changes May 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pre commit additions #3

Pre commit additions #3

AoifeHughes commented Apr 30, 2024

AoifeHughes commented Apr 30, 2024

AoifeHughes commented Apr 30, 2024 •

edited

Loading

AoifeHughes commented May 19, 2024

AoifeHughes commented May 19, 2024

HChughtai left a comment

HChughtai May 22, 2024

HChughtai May 22, 2024

HChughtai May 22, 2024

HChughtai May 22, 2024

HChughtai May 22, 2024

HChughtai May 22, 2024

HChughtai May 22, 2024

HChughtai May 22, 2024

HChughtai May 22, 2024

	- uses: actions/setup-python@v4
	- uses: actions/setup-python@v5

	pre-commit run --all-files
	pre-commit run --all-files --color always --verbose

Pre commit additions #3

Are you sure you want to change the base?

Pre commit additions #3

Conversation

AoifeHughes commented Apr 30, 2024

AoifeHughes commented Apr 30, 2024

AoifeHughes commented Apr 30, 2024 • edited Loading

AoifeHughes commented May 19, 2024

AoifeHughes commented May 19, 2024

HChughtai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AoifeHughes commented Apr 30, 2024 •

edited

Loading