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

PTV-1888 #192

Open
wants to merge 41 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
5ce5dc2
add a devcontainer [PTV-1888]
eapearson Aug 31, 2023
b168c68
update Dockerfiles to use python 3.11.5, bullseye, and fix os depend…
eapearson Aug 31, 2023
dee74d3
add development tools [PTV-1888]
eapearson Aug 31, 2023
7b070cd
ignore jetbrains idea directory and temp dir [PTV-1888]
eapearson Aug 31, 2023
d4905ac
split Python dependencies into service and development, and update th…
eapearson Aug 31, 2023
a7fa5d4
fix flaky tests [PTV-1888]
eapearson Aug 31, 2023
34a4474
fix local config [PTV-1888]
eapearson Sep 1, 2023
ffdb749
add docker plugin to devcontainer [PTV-1888]
eapearson Sep 1, 2023
10cc772
put some work into docs [PTV-1888]
eapearson Sep 1, 2023
d8f2937
Merge remote-tracking branch 'origin/develop' into PTV-1888
eapearson Sep 1, 2023
f23e925
update release notes, update GHA test to use python 3.11.5
eapearson Sep 1, 2023
aba6c7e
fix sonar warning [PTV-1888]
eapearson Sep 1, 2023
e5c1915
code quality gatekeeper changes [PTV-1888]
eapearson Sep 1, 2023
62daccb
run tests in our container [PTV-1888]
eapearson Sep 2, 2023
b68c168
run tests in our container [PTV-1888]
eapearson Sep 2, 2023
0f90a22
improve script invocation [PTV-1888]
eapearson Sep 2, 2023
b79f478
somehow wasn't bookworm
eapearson Sep 2, 2023
153f852
don't run as regular user. [PTV-1888]
eapearson Sep 2, 2023
b19cd9c
be more tolerant of different st_mtime implementations [PTV-1888]
eapearson Sep 2, 2023
806dca8
erik, you are too tired [PTV-1888]
eapearson Sep 2, 2023
f043228
clarify test condition comment [PTV-1888]
eapearson Sep 5, 2023
c5ee5a0
add markdown lint, fix linting errors in all markdown files [PTV-1888]
eapearson Sep 5, 2023
ccb7660
remove old, unused travis config [PTV-1888]
eapearson Sep 5, 2023
09b36e4
top level scripts have moved to the scripts directory [PTV-1888]
eapearson Sep 5, 2023
766872d
fix typos [PTV-1888]
eapearson Sep 5, 2023
4c38f0d
fix doc [PTV-1888]
eapearson Sep 5, 2023
d710581
adjust mtime test precision.
eapearson Sep 5, 2023
c2f9c16
remove spaces (codacy) [PTV-1888]
eapearson Sep 5, 2023
1ad078a
markdown formatting [PTV-1888]
eapearson Sep 5, 2023
d384ec3
omit line for bandit/codacy not sonarcloud (codacy) [PTV-1888]
eapearson Sep 5, 2023
8b6f62e
add openssh-client so can push to github inside devcontainer [PTV-1888]
eapearson Sep 5, 2023
b0485e3
okay, ignore in sonarcloud too [PTV-1888]
eapearson Sep 5, 2023
01dc36a
bullseye -> bookworm [PTV-1888]
eapearson Sep 5, 2023
ae1fc76
revert globus sdk version bump [PTV-1888]
eapearson Sep 12, 2023
14602e2
fix typos [PTV-1899]
eapearson Sep 12, 2023
150421e
refactor Dockerfile to support dev and prod workflows [PTV-1888]
eapearson Sep 15, 2023
34a4636
consolidate tool config into pyproject [PTV-1888]
eapearson Sep 15, 2023
2090307
update python dependencies [PTV-1888]
eapearson Sep 15, 2023
73b0266
set pythonpath [PTV-1888]
eapearson Sep 15, 2023
34f6f36
apply black and isort after new configs [PTV-1888]
eapearson Sep 15, 2023
4d4dc38
remove extra comma [PTV-1888]
eapearson Sep 15, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions .devcontainer/Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
FROM python:3.11.5-slim-bookworm
# -----------------------------------------
# We use the "stable" debian release; python does not seem to release current
# versions on LTS debian releases.

# We don't upgrade, and we pin all os dependency versions
RUN apt-get update && \
apt-get install -y --no-install-recommends zip=3.0-13 unzip=6.0-28 bzip2=1.0.8-5+b1 libmagic-dev=1:5.44-3 htop=3.2.2-2 wget=1.21.3-1+b2 git=1:2.39.2-1.1 openssh-client=1:9.2p1-2 && \
apt-get clean && \
rm -rf /var/lib/apt/lists/*

SHELL ["/bin/bash", "-c"]

RUN mkdir -p /app

WORKDIR /app

# Standard simplified python setup.
COPY ./requirements.txt /app/requirements.txt
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should use pipfile instead?

Copy link
Author

Choose a reason for hiding this comment

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

The existing documentation says that the requirements.txt file is the one that is used, and that the Pipfile is there and (possibly) maintained for those who like to use it for development.
Personally, I have never seen a backup package manager in a repo like this, and wish it weren't so. In some JS codebases we might have had npm and bower in the same repo, but that was a transition from an old package manager to a newer one.
I'd be ok with Pipfile, though I think if we switch the package manager Poetry would be a better choice than pipenv.
IMO It would be nice for KBase to support just 2 Python package managers - plain pip with requirements.txt (since it is so common) for legacy projects (and it is the one supported by kb-sdk), and Poetry for new repos.

COPY ./requirements_dev.txt /app/requirements_dev.txt
RUN python -m pip install "pip==23.2.1" && pip install -r /app/requirements.txt -r /app/requirements_dev.txt

ENV PATH="/app:${PATH}"
39 changes: 39 additions & 0 deletions .devcontainer/devcontainer.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// For format details, see https://aka.ms/devcontainer.json. For config options, see the
// README at: https://github.com/devcontainers/templates/tree/main/src/docker-existing-dockerfile
{
"name": "staging service devcontainer",
"dockerComposeFile": [
"docker-compose.yml"
],
// The 'service' property is the name of the service for the container that VS Code should
// use. Update this value and .devcontainer/docker-compose.yml to the real service name.
"service": "staging_service",
// "workspaceMount": "source=${localWorkspaceFolder},target=/workspace,type=bind,consistency=cached",
// The optional 'workspaceFolder' property is the path VS Code should open by default when
// connected. This is typically a file mount in .devcontainer/docker-compose.yml
"workspaceFolder": "/workspace",
bio-boris marked this conversation as resolved.
Show resolved Hide resolved
"customizations": {
"vscode": {
"extensions": [
"ms-python.python",
"tamasfe.even-better-toml",
"mhutchie.git-graph",
"ms-python.black-formatter",
"ms-python.flake8",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest moving to Ruff -- it replaces most/all flake8 packages as well as adding its own rules, has an auto fix setting, and is much quicker than flake8. There's a VSCode integration for it: https://github.com/astral-sh/ruff-vscode

Copy link
Collaborator

Choose a reason for hiding this comment

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

On second thoughts, having finished going through the PR, leave Ruff for another PR. Not worth waiting to merge this one for a "nice to have".

"mikoz.autoflake-extension",
"ms-azuretools.vscode-docker",
"DavidAnson.vscode-markdownlint"
]
}
}
// Features to add to the dev container. More info: https://containers.dev/features.
// "features": {},
// Use 'forwardPorts' to make a list of ports inside the container available locally.
// "forwardPorts": [],
// Uncomment the next line to run commands after the container is created.
// "postCreateCommand": "cat /etc/os-release",
// Configure tool-specific properties.
// "customizations": {},
// Uncomment to connect as an existing user other than the container default. More info: https://aka.ms/dev-containers-non-root.
// "remoteUser": "devcontainer"
}
24 changes: 24 additions & 0 deletions .devcontainer/docker-compose.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
version: '3.6'
networks:
kbase-dev:
name: kbase-dev
services:
staging_service:
build:
context: ..
dockerfile: .devcontainer/Dockerfile

image: kbase/staging_server_devcontainer:dev

container_name: staging_server_devcontainer
dns: 8.8.8.8
volumes:
- ..:/workspace:cached
bio-boris marked this conversation as resolved.
Show resolved Hide resolved
networks:
- kbase-dev
# Required for a devcontainer -- keeps the container running.
# Don't worry, our main interaction with the container is through the
# VSC terminal, which for a devcontainer opens a shell within the
# container.
command: /bin/sh -c "while sleep 1000; do :; done"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to also open a debug server with this somehow and connect your IDE debugger into this with VSCODE? I believe it can be done in pycharm. Just wondering.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, pretty sure, although I haven't tried. When using a devcontainer you are actually running VSC in the container, so it is like running on whatever the platform is - in this case debian.

Unfortunately jetbrains is still catching up with devcontainers. They have initial support (and they have other similar things that are not as clean and smooth), but it is not very complete so can't be used for real work.

I typically run the service in development mode, and bang away at endpoints from an http tool, and then the front end, and use either a terminal in the devcontainer or the host tools to run tests.

In later work (the next PR), I have used these tools, esp. mypy, more extensively and will have a few changes.

I find it effective to have cli tool support via a tool container and in the devcontainer, and to have those same tools available via VSC (or pycharm) plugins. It is a some work to set up, a pain in a codebase that is big and has not used them, and not insignificant maintenance, but I think that is the price for Python development these days.

(Don't get me started on all the extra work required to keep a Python codebase healthy; but I think ruff will be a good substitute for some of those tools and others not included here.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

image

Looks like env doesn't work on my machine

Upgraded my docker-compose still doesn't work

Docker Compose version 2.21.0

    environment:
      KB_DEPLOYMENT_CONFIG: ./deployment/conf/local.cfg
      FILE_LIFETIME: 90

might work


24 changes: 5 additions & 19 deletions .github/workflows/run_tests.yml
Original file line number Diff line number Diff line change
@@ -1,26 +1,12 @@
# This workflow will install Python dependencies, run tests and lint with a single version of Python
# For more information see: https://help.github.com/actions/language-and-framework-guides/using-python-with-github-actions

name: Run Staging Service Tests

on:
[pull_request]

jobs:
build:

runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v2
- name: Set up Python 3.11
uses: actions/setup-python@v2
with:
python-version: 3.11.4
- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install -r requirements.txt
- name: Test with pytest
run: |
bash run_tests.sh
- name: Checkout the repo
uses: actions/checkout@v3

Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM

- name: Run the tests
run: ./development/scripts/run scripts/run_tests.sh
8 changes: 7 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -107,4 +107,10 @@ data/
.DS_Store
Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM

.virtualenvs/
test.env
run_tests_single.sh
run_tests_single.sh
_attic

# IDEs

# jetbrains
.idea
10 changes: 10 additions & 0 deletions .markdownlint.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
MD013:
# Matches black line length for python code.
line_length: 88
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should consider having a larger line length? I believe most of us have large monitors nowadays. I kind of take it to the extreme and like 180 chars so I can keep string messages on the same line!

# some long examples would break the line length and are awkward to rearrange
code_blocks: false

# We don't want the annoying warnings when there are duplicated non-sibling headers as
# many types of docs have a pattern for headings that is repeated throughout the document.
MD024:
siblings_only: true
16 changes: 0 additions & 16 deletions .travis.yml

This file was deleted.

6 changes: 3 additions & 3 deletions Dockerfile
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
FROM python:3.11.4-slim-buster
FROM python:3.11.5-slim-bookworm
Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM

# -----------------------------------------
RUN mkdir -p /kb/deployment/lib
RUN apt-get update && \
apt-get install -y --no-install-recommends zip=3.0-11+b1 unzip=6.0-23+deb10u3 bzip2=1.0.6-9.2~deb10u2 libmagic-dev=1:5.35-4+deb10u2 htop=2.2.0-1+b1 wget=1.20.1-1.1 && \
apt-get install -y --no-install-recommends zip=3.0-13 unzip=6.0-28 bzip2=1.0.8-5+b1 libmagic-dev=1:5.44-3 htop=3.2.2-2 wget=1.21.3-1+b2 && \
apt-get clean && \
rm -rf /var/lib/apt/lists/*

COPY ./requirements.txt /requirements.txt
RUN python -m pip install "pip==23.1.2" && pip install -r /requirements.txt && rm /requirements.txt
RUN python -m pip install "pip==23.2.1" && pip install -r /requirements.txt && rm /requirements.txt

COPY ./globus.cfg /etc/globus.cfg
RUN touch /var/log/globus.log && chmod 777 /var/log/globus.log
Expand Down
Loading