Skip to content

feat: Migrate JWA test-related workflows from kubeflow/kubeflow to notebooks-v1 branch #587 #599

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: notebooks-v1
Choose a base branch
from
+256 −5
Open

feat: Migrate JWA test-related workflows from kubeflow/kubeflow to notebooks-v1 branch #587 #599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
38 changes: 38 additions & 0 deletions .github/workflows/jwa_backend_unittests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
name: JWA Backend Tests
on:
pull_request:
paths:
- components/crud-web-apps/jupyter/backend/**
- releasing/version/VERSION
branches:
- main
- notebooks-v1
- 'v*-branch'

jobs:
run-backend-unittests:
name: Unittests
runs-on: ubuntu-22.04
steps:
- name: Checkout
uses: actions/checkout@v4

- name: Install KinD
run: ./components/testing/gh-actions/install_kind.sh

- name: Create KinD Cluster
run: kind create cluster --config components/testing/gh-actions/kind-1-25.yaml

- uses: actions/setup-python@v5
with:
python-version: "3.7"
Copy link
Member

Choose a reason for hiding this comment

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

May we use 3.12? 3.7 is EOL.

Copy link
Contributor

Choose a reason for hiding this comment

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

@juliusvonkohout

as outlined in my #kubeflow-notebooks Slack message - I'd prefer for this initial PR to migrate the workflows - that we try to adhere as close as possible to the kubeflow/kubeflow implementation.

Your feedback/insights totally spot on - I'd just prefer a follow up PR once we have notebooks-v1 branch of kubeflow/notebooks at feature parity .


- name: Setup Python environment
run: |
cd components/crud-web-apps/jupyter/backend
make install-deps
Copy link
Member

Choose a reason for hiding this comment

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

make install-dependencies i would prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

see: #599 (comment)


- name: Run unittests
run: |
cd components/crud-web-apps/jupyter/backend
make unittest
98 changes: 98 additions & 0 deletions .github/workflows/jwa_frontend_tests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
name: JWA Frontend Tests
on:
pull_request:
paths:
- components/crud-web-apps/jupyter/frontend/**
- releasing/version/VERSION
branches:
- main
- notebooks-v1
- 'v*-branch'

jobs:
frontend-format-linting-check:
name: Check code format and lint
runs-on: ubuntu-22.04

steps:
- name: Check out code
uses: actions/checkout@v4

- name: Setup Node
uses: actions/setup-node@v4
with:
node-version: 16

- name: Check frontend formatting
run: |
cd components/crud-web-apps/jupyter/frontend
npm i
npm run format:check

- name: Check frontend code linting
run: |
cd components/crud-web-apps/jupyter/frontend
npm i
npm run lint-check
Comment on lines +13 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize you simply moved this code over from kubeflow/kubeflow - but the inefficiency present here is just too much for me to ignore 🤣

I am deliberately ignoring other steps in this workflow (as due to kubeflow-common-lib they are a little more complicated) - but for this specific job - I feel like we should:

  • define working-directory (as its the only (singular) directory in this job that matters)
  • use npm ci for more deterministic builds
  • only run npm ci once (doing it before the format and lint heck)
Suggested change
frontend-format-linting-check:
name: Check code format and lint
runs-on: ubuntu-22.04
steps:
- name: Check out code
uses: actions/checkout@v4
- name: Setup Node
uses: actions/setup-node@v4
with:
node-version: 16
- name: Check frontend formatting
run: |
cd components/crud-web-apps/jupyter/frontend
npm i
npm run format:check
- name: Check frontend code linting
run: |
cd components/crud-web-apps/jupyter/frontend
npm i
npm run lint-check
frontend-format-linting-check:
name: Check code format and lint
runs-on: ubuntu-22.04
defaults:
run:
working-directory: components/crud-web-apps/jupyter/frontend
steps:
- name: Check out code
uses: actions/checkout@v4
- name: Setup Node
uses: actions/setup-node@v4
with:
node-version: 16
- name: Install dependencies
run: npm ci
- name: Check frontend formatting
run: npm run format:check
- name: Check frontend code linting
run: npm run lint-check


frontend-unit-tests:
runs-on: ubuntu-22.04
name: Unit tests
steps:
- name: Checkout
uses: actions/checkout@v4

- name: Setup node version to 16
uses: actions/setup-node@v4
with:
node-version: 16

- name: Install Kubeflow common library dependecies
run: |
cd components/crud-web-apps/common/frontend/kubeflow-common-lib
npm i
npm run build
npm link ./dist/kubeflow

- name: Install JWA dependencies
run: |
cd components/crud-web-apps/jupyter/frontend
npm i
npm link kubeflow

- name: Run unit tests
run: |
cd components/crud-web-apps/jupyter/frontend
npm run test:prod

run-ui-tests:
name: UI tests with Playwright
runs-on: ubuntu-22.04
steps:
- name: Checkout
uses: actions/checkout@v4

- name: Setup node version to 16
uses: actions/setup-node@v4
with:
node-version: 16

- name: Install Kubeflow common library dependecies
run: |
cd components/crud-web-apps/common/frontend/kubeflow-common-lib
npm i
npm run build
npm link ./dist/kubeflow

- name: Install JWA dependencies
run: |
cd components/crud-web-apps/jupyter/frontend
npm i
npm link kubeflow

- name: Serve UI & run Playwright tests in Chrome and Firefox
run: |
cd components/crud-web-apps/jupyter/frontend
npm run serve > serve.log 2>&1 & npx wait-on http://localhost:4200
npx playwright install
npm run ui-test-ci-all
68 changes: 68 additions & 0 deletions .github/workflows/jwa_integration_test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
name: JWA Integration Test
on:
pull_request:
paths:
- components/crud-web-apps/jupyter/**
- components/crud-web-apps/common/**
- releasing/version/VERSION
branches:
- main
- notebooks-v1
- 'v*-branch'

concurrency:
group: ${{ github.workflow }}-${{ github.ref }}-${{ github.event.number }}
cancel-in-progress: true

env:
IMG: ghcr.io/kubeflow/notebooks/jupyter-web-app
TAG: integration-test

jobs:
build:
runs-on: ubuntu-22.04
steps:
- name: Checkout
uses: actions/checkout@v4

- name: Setup Docker Buildx
uses: docker/setup-buildx-action@v3

- name: Build JWA Image
run: |
cd components/crud-web-apps/jupyter
make docker-build-multi-arch

- name: Install KinD
run: ./components/testing/gh-actions/install_kind.sh

- name: Create KinD Cluster
run: kind create cluster --config components/testing/gh-actions/kind-1-25.yaml

- name: Load Image into KinD Cluster
run: |
kind load docker-image "${IMG}:${TAG}"

- name: Install kustomize
run: ./components/testing/gh-actions/install_kustomize.sh

- name: Install Istio
run: ./components/testing/gh-actions/install_istio.sh

- name: Build & Apply manifests
run: |
cd components/crud-web-apps/jupyter/manifests
kubectl create ns kubeflow

export CURRENT_IMAGE="${IMG}"
export PR_IMAGE="${IMG}:${TAG}"

# escape "." in the image names, as it is a special characters in sed
export CURRENT_IMAGE=$(echo "$CURRENT_IMAGE" | sed 's|\.|\\.|g')
export PR_IMAGE=$(echo "$PR_IMAGE" | sed 's|\.|\\.|g')

kustomize build overlays/istio \
| sed "s|${CURRENT_IMAGE}:[a-zA-Z0-9_.-]*|${PR_IMAGE}|g" \
| kubectl apply -f -

kubectl wait pods -n kubeflow -l app=jupyter-web-app --for=condition=Ready --timeout=300s
47 changes: 47 additions & 0 deletions .github/workflows/jwa_multi_arch_test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
name: JWA Multi-Arch Build Test
on:
pull_request:
paths:
- components/crud-web-apps/jupyter/**
- components/crud-web-apps/common/**
- releasing/version/VERSION
branches:
- main
- notebooks-v1
- 'v*-branch'

concurrency:
group: ${{ github.workflow }}-${{ github.ref }}-${{ github.event.number }}
cancel-in-progress: true

env:
IMG: ghcr.io/kubeflow/notebooks/jupyter-web-app
PLATFORMS: linux/amd64,linux/ppc64le,linux/arm64

jobs:
build:
runs-on: ubuntu-22.04
steps:
- name: Checkout
uses: actions/checkout@v4

- name: Setup QEMU
uses: docker/setup-qemu-action@v3

- name: Setup Docker Buildx
uses: docker/setup-buildx-action@v3

- name: Build multi-arch images
uses: docker/build-push-action@v5
with:
context: components/crud-web-apps/
file: components/crud-web-apps/jupyter/Dockerfile
platforms: ${{ env.PLATFORMS }}
push: false
load: false
Comment on lines +40 to +41
Copy link
Contributor

Choose a reason for hiding this comment

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

Both the push and load parameters are defaulted to false - so I think we should simply omit them from this action declaration.

Suggested change
push: false
load: false

tags: |
${{ env.IMG }}:${{ github.sha }}
${{ env.IMG }}:latest
Comment on lines +42 to +44
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't really need to define multiple tags here... particularly as we aren't loading and/or pushing these images.. so I'd keep it simple/basic

Suggested change
tags: |
${{ env.IMG }}:${{ github.sha }}
${{ env.IMG }}:latest
tags: ${{ env.IMG }}:${{ github.sha }}

cache-from: type=gha
cache-to: type=gha,mode=max
Comment on lines +45 to +46
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd maybe opt to NOT implement caching - at least initially.

Rationale as follows:

  • we didn't do it in the past
  • we don't plan on making a lot of changes to notebooks-v1 moving forward (after migration)
  • adds uncertainty/perceived risk of having bad/stale cache

With notebooks-v1 and notebooks-v2 both being in the same repo - and as such subjected to the same constraints - if we were going to look into caching for performance/optimizations - I'd rather us focus on notebooks-v2 exclusively.. i.e. I don't want notebooks-v1 cache artifacts eating into our "quota" if/when we want to explore caching for the notebooks-v2 space.

Suggested change
cache-from: type=gha
cache-to: type=gha,mode=max


3 changes: 1 addition & 2 deletions components/crud-web-apps/jupyter/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
IMG ?= ghcr.io/kubeflow/kubeflow/jupyter-web-app
IMG ?= ghcr.io/kubeflow/notebooks/jupyter-web-app
TAG ?= $(shell git describe --tags --always --dirty)
DOCKERFILE ?= jupyter/Dockerfile
ARCH ?= linux/amd64
Expand All @@ -14,7 +14,6 @@ docker-push:
docker-build-multi-arch: ## Build multi-arch docker images with docker buildx
cd ../ && docker buildx build --load --platform ${ARCH} --tag ${IMG}:${TAG} -f ${DOCKERFILE} .


.PHONY: docker-build-push-multi-arch
docker-build-push-multi-arch: ## Build multi-arch docker images with docker buildx and push to docker registry
cd ../ && docker buildx build --platform ${ARCH} --tag ${IMG}:${TAG} --push -f ${DOCKERFILE} .
Expand Down
2 changes: 1 addition & 1 deletion components/crud-web-apps/jupyter/manifests/base/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ spec:
spec:
containers:
- name: jupyter-web-app
image: ghcr.io/kubeflow/kubeflow/jupyter-web-app
image: ghcr.io/kubeflow/notebooks/jupyter-web-app
ports:
- containerPort: 5000
volumeMounts:
Expand Down
4 changes: 2 additions & 2 deletions components/crud-web-apps/jupyter/manifests/base/kustomization.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ commonLabels:
app: jupyter-web-app
kustomize.component: jupyter-web-app
images:
- name: ghcr.io/kubeflow/kubeflow/jupyter-web-app
newName: ghcr.io/kubeflow/kubeflow/jupyter-web-app
- name: ghcr.io/kubeflow/notebooks/jupyter-web-app
newName: ghcr.io/kubeflow/notebooks/jupyter-web-app
newTag: latest
# We need the name to be unique without the suffix because the original name is what
# gets used with patches
Expand Down
1 change: 1 addition & 0 deletions releasing/version/VERSION
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
latest
Loading