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.

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

Updates to github workflows for PDO #504

Merged
merged 2 commits into from
Oct 30, 2024
Merged

Updates to github workflows for PDO #504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5cdd803
Updates to github workflows for PDO
cmickeyb Oct 22, 2024
9e2b471
add documentation to workflows
cmickeyb Oct 30, 2024
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
71 changes: 71 additions & 0 deletions .github/workflows/configured_test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
#
# SPDX-License-Identifier: Apache-2.0
#

# This workflow is intended to provide an interactive way of configuring
# PDO tests. Common configuration variables can be set interactively to
# debug differences between local and github.

name: Run specific PDO tests
on:
workflow_dispatch:
inputs:
interpreter:
description: 'Interpreter'
required: true
default: 'wawaka'
type: choice
options:
- wawaka
- wawaka-opt
logLevel:
description: 'Log level'
required: true
default: 'warning'
type: choice
options:
- debug
- info
- warning
memoryConfiguration:
description: 'Interpreter memory configuration'
required: false
default: MEDIUM
type: choice
options:
- SMALL
- MEDIUM
- LARGE

jobs:
pdo_specific_tests:
name: Run specific PDO tests
runs-on: ubuntu-22.04
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be another parameter (input)? Although admittedly as everything is done in docker, if we would parameterize that it probably would be better to parameterize docker's UBUNTU_VERSION build argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. if i can actually use the variables from the dispatch i will add this. it will probably have to be fixed in a later PR though since i have no way to test it right now.


steps:
- name: Check out repo
uses: actions/checkout@v4
with:
submodules: recursive
fetch-depth: 0
fetch-tags: true

- name: Display branch name
run: |
echo "Building branch $GITHUB_HEAD_REF"
echo PDO VERSION is $(bin/get_version)

- name: Build and run tests
env:
PDO_INTERPRETER: ${{ inputs.interpreter }}
PDO_LOG_LEVEL: ${{ inputs.logLevel }}
PDO_MEMORY_CONFIG: ${{ inputs.memoryConfiguration }}
PDO_DEBUG_BUILD: 1
run: |
# The creation of a dummy branch is necessary for the CI tests
# to work on PRs. Based on empirical results, in the absence of
# this command, CI tests work on the main branch and on local
# branches. However, they fail as a PR is created.
git checkout -b ci-test-branch
. build/common-config.sh
make -C docker test
63 changes: 63 additions & 0 deletions .github/workflows/docker.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
#
# SPDX-License-Identifier: Apache-2.0
#

name: Build and Push PDO Docker Images

on:
workflow_dispatch:

pull_request:
types: [closed]
Copy link
Contributor

@g2flyer g2flyer Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Before seeing below if, i stumbled over this [closed] and the lack of something like a type [merged]. Googling a bit i found https://github.com/orgs/community/discussions/26724#discussioncomment-3253093 which proposes an arguably simpler solution to achieve the same effect: on: push: branches: - main

Oops, i notice the same discussion later also mentions your pattern. I didn't see though any problem with above pattern which still seems moderately simpler/more robust (e.g., you don't have to match other triggers in the if clause ..)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually

  1. The comment at the top of that issue is actually wrong (you can detect a merged PR)
  2. Even in that series of responses, the conclusion is to use the pull_request event with a check for merged

I'm going to leave this as is.

branches: [main]

jobs:

docker_build:

if: >
github.event.name == 'workflow_dispatch' ||
github.event.name == 'pull_request' && github.event.pull_request.merged == true
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now we would generate the dockers for every PR, whether it is a version upgrade or not. But i guess there is no easy way to test whether this is a version upgrade PR? That said, as long as storage is not an issue, i guess there is loittle cost for doing it on all PRs (the actual build should be "free" if github caching works properly as the CI already should have built them?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree. but remember that the version is updated with every commit. i think there is a question that we need to talk through about when to trigger the action. i don't think this is an unreasonable starting point. i would prefer to get this version working and then come back and address the trigger conditions.

name: Build PDO Images
runs-on: ubuntu-22.04

steps:
- name: Check out repo
uses: actions/checkout@v4
with:
submodules: recursive
fetch-depth: 0
fetch-tags: true

- name: Display branch name
run: |
echo "Building branch images for $GITHUB_HEAD_REF"
echo PDO VERSION is $(bin/get_version)
echo "PDO_VERSION=$(bin/get_version)" >> $GITHUB_ENV
echo "EVENT NAME: ${{ github.event.name }}"
echo "MERGED: ${{ github.event.pull_request.merged }}"

- name: Build Docker Images
env:
PDO_INTERPRETER: wawaka
PDO_LOG_LEVEL: warning
run: |
git checkout -b ci-test-branch
. build/common-config.sh
make -C docker

- name: Login to the ghcr.io Container Registry
uses: docker/login-action@v2
with:
registry: ghcr.io
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}

- name: Tag and push the images
run: |
for image in pdo_services pdo_ccf pdo_client
do
docker image tag ghcr.io/{{ github.repository_owner }}/$image:$PDO_VERSION
docker image tag ghcr.io/{{ github.repository_owner }}/$image:latest
docker image push --all-tags ghcr.io/{{ github.repository_owner }}/$image
done
32 changes: 12 additions & 20 deletions .github/workflows/ci.yaml → .github/workflows/full_test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,19 @@
# SPDX-License-Identifier: Apache-2.0
#

name: PDO CI
on: [pull_request, push]
# This workflow is intended to be used as a validity test for any
# pull request. That is, this is a minimal functionality that must
# be successfully executed prior to merging a pull request. Note
# that this can be overridden by adding '[skip ci]' in the commit
# name. This should not be done on the main PDO branch.

name: Run full PDO tests
on: [ pull_request ]

jobs:
pdo_ci:
pdo_full_tests:
if: "!contains(github.event.commits[0].message, '[skip ci]')"
name: PDO CI Job
name: Run full PDO tests
runs-on: ubuntu-22.04

strategy:
Expand All @@ -25,11 +32,10 @@ jobs:

- name: Display branch name
run: |
echo "Building branch $GITHUB_HEAD_REF"
echo PDO VERSION is $(bin/get_version)
echo "BRANCH is $GITHUB_HEAD_REF"

- name: Build and run tests
if: "!contains(github.event.commits[0].message, '[debug]')"
Copy link
Contributor

Choose a reason for hiding this comment

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

The old way of triggering debug builds clearly was hacky. But maybe would be good to add a comment to configured_test.yml and the possibility to manually trigger now parameterized builds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I will document it better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small amount of documentation added.

env:
PDO_INTERPRETER: ${{ matrix.interpreter }}
PDO_LOG_LEVEL: warning
Expand All @@ -41,17 +47,3 @@ jobs:
git checkout -b ci-test-branch
. build/common-config.sh
make -C docker test

- name: Build and run tests (DEBUG MODE)
if: "contains(github.event.commits[0].message, '[debug]')"
env:
PDO_INTERPRETER: ${{ matrix.interpreter }}
PDO_LOG_LEVEL: debug
run: |
# The creation of a dummy branch is necessary for the CI tests
# to work on PRs. Based on empirical results, in the absence of
# this command, CI tests work on the main branch and on local
# branches. However, they fail as a PR is created.
git checkout -b ci-test-branch
. build/common-config.sh
make -C docker test