canonical / bundle-kubeflow

Charmed Kubeflow
Apache License 2.0
104 stars 50 forks source link

Scheduled AKS and EKS ci doesn't work due to empty version #1119

Closed orfeas-k closed 1 month ago

orfeas-k commented 1 month ago

Bug Description

When scheduled, the workflows do not work because they are given an empty bundle_version.

When scheduled, there are no inputs (as when run from a workflow_dispatch). IIUC, this results in the workflow to call the parse_versions.py script with an input "". The script expects no argument in order to fail, thus it doesn't fail and instead outputs bundle_versions=[""].

We need to refactor the preprocess script/job in order to account for cases without inputs.

To Reproduce

trigger without any inputs e.g. pull_request event or wait for scheduled action

Environment

GH runners

Relevant Log Output

n/a

Additional Context

No response

syncronize-issues-to-jira[bot] commented 1 month ago

Thank you for reporting us your feedback!

The internal ticket has been created: https://warthogs.atlassian.net/browse/KF-6457.

This message was autogenerated

orfeas-k commented 1 month ago

I see this was a design decision in https://github.com/canonical/bundle-kubeflow/pull/1101

the default value in the parse_versions() function has been removed, so calling the script without any arguments raises an appropriate exception.

This however is not in sync with our expected scheduled behaviour, where we want to run the workflows for latest + the two supported versions.

orfeas-k commented 1 month ago

After a quick sync with @mvlassis, we both agreed that default values should only be set in the workflow and not the script. With that in mind, we came up with the two options:

  1. Adding a step in preprocess input job, that will run only on schedule and then this will be passed to the script as an argument, when input.kf_version is empty:

      - name: Set env
        if: ${{ github.event_name == 'schedule' }}
        run: echo "BUNDLE_VERSION=1.9" >> $GITHUB_ENV
    
      - name: Process bundle versions
        id: process_bundle_versions
        run: python scripts/gh-actions/parse_versions.py "${{ inputs.bundle_version || env.BUNDLE_VERSION }}"
  2. Modify the workflows to enable workflow_call with the same inputs that workflow_dispatch has and then create a separate e.g. deploy-to-a/eks-schedule.yaml or scheduled.yaml workflow that will call the deploy-to-a/eks.yaml with the appropriate inputs (setting the default there).
DnPlas commented 1 month ago

I'm not sure I'd like to go with (2), it seems to be adding more maintenance to us. I think I prefer going with a hybrid of (1), but first, I think there are a couple things going on here:

  1. The python script is not raising when expected. I have noticed that the lines checking the length of the "input" is slightly incorrect. Right now we are doing the following in L15-L16:
    if len(sys.argv) < 1: # <--- this will always be False, as sys.argv[0] is always the name of the script
        raise Exception("No bundle versions given as input.")

In a pdb session:

$ python3 scripts/gh-actions/parse_versions.py ""
-> if len(sys.argv) < 1:
(Pdb) sys.argv
['scripts/gh-actions/parse_versions.py', '']
(Pdb) len(sys.argv)
2
(Pdb) sys.argv[0]
'scripts/gh-actions/parse_versions.py'

We have to change this to either sys.argv[1] or, slightly fancier, refactor with argparse.

  1. Since the ${{ inputs.bundle_version }} will be null for the scheduled workflow run, we don't need to run anything from the preprocess input job on schedule.

Proposal

I think we can:

So it would look like:

# please be mindful the syntax can be slightly wrong

jobs:
  preprocess-input:
  if: ${{ github.event_name == 'wf dispatch' }} # we should check the actual name
  ...

  generate-bundle-versions-on-sched:
  if: ${{ github.event_name == 'schedule' }}
  run: |
    bundle_versions=$(yq '. | keys' .github/dependencies.yaml -o=json) # this gives you a json array
    echo "bundle_versions=$" >> "$GITHUB_OUTPUT"

  deploy-ckf-to-aks:
    runs-on: ubuntu-22.04
    strategy:
      matrix:
        bundle_version: ${{ fromJSON(jobs.preprocess-input.outputs.processed_bundle_versions) || fromJSON(jobs.generate-bundle-versions-on-sched.outputs.bundle_versions) }}

thoughts?

mvlassis commented 1 month ago

@DnPlas That is a great suggestion, I will be implementing this (and crediting you :))