search

canonical / bundle-kubeflow

Charmed Kubeflow
Apache License 2.0
104 stars 50 forks source link

Workflows for ROCK repositories #692

Closed i-chvets closed 9 months ago

i-chvets commented 1 year ago

Description

ROCK repositories require workflows that will automate ROCK changes/update/patching approval and publishing process.

Design

Whenever rockracft.yaml file is updated due to version update, security patches, and/or changes related to new features and best practices a pull request is created. When pull request is created a number of actions need to be executed to validate changes and summarize security posture of new ROCK. When changes are approved, rockcraft.yaml is merged and should be published to ROCK registry. Each ROCKs directory should have tox.ini file with unit and integration environments setup to be used in workflows.

The above process aligns with CVE management spec steps that are required when patching ROCKs:

h. Create PR, on pull request an automated workflow will build and scan updated ROCK images. Integration tests of corresponding charm are also executed in this workflow. i. Attach summary of scan report of updated ROCK image produced by workflow to PR, GH issue, and/or Jira. j. When PR is approved and merged, an automated workflow will publish the updated ROCK image.

Tests

Unit tests for each ROCK should be setup in tests/ directory and should be invoked via tox -e unit Integration tests for corresponding charm are used for integration testing of ROCKs. These tests should be invoiked via tox -e integration. Main process for executing integration tests is:

Pull request automation

ROCK repositories require on pull request automation that will do the following on every open pull request:

Merge automation

ROCK repositories require on marge automation that will do the following on every merge

List of repositories:

i-chvets commented 1 year ago

Discussion:

These kind of commands can be extracted to the workflow file so that we can have that in the reusable ci: Feat notebook controller test by i-chvets · Pull Request #39 · canonical/kubeflow-rocks

Replacing container image names can be done better if we use a json file for saving all image names, and then replacing from there. We will have two places to edit: default-images.json and metadata.yaml Feat notebook controller test by i-chvets · Pull Request #39 · canonical/kubeflow-rocks

Nice idea: trigger integration tests from the rocks repository to run in the remote charm repositories Feat notebook controller test by i-chvets · Pull Request #39 · canonical/kubeflow-rocks check Triggering a workflow - GitHub Docs

We have tox.ini inside every rock directory because we have some knowledge specific to that rock, in particular the smoke tests, which check that the rock has a file inside them. Having an ini file inside will ensure that we can do a tox -e smoke-test on a single rock.

We can have a check for knowing which rockcraft.yaml changed and only run the whole thing for only the rocks that changed -> Changed Files - GitHub Marketplace

We have to make a distinction between top level integration tests and individual smoke tests

Publish workflow for rocks [Feature Request] Publish rock to Github's Container Registry · Issue #5 · canonical/craft-actions

We have decided to work on these improvements to the current proposal.

i-chvets commented 1 year ago

@DnPlas @kimwnasptd I've added our discussion as a comment and haven't update it in descrition yet.

Here are some questions that we need to address:

DnPlas commented 1 year ago

@DnPlas @kimwnasptd I've added our discussion as a comment and haven't update it in descrition yet.

Here are some questions that we need to address:

  • Without integration tests in ROCK repository how do we ensure ROCK is acceptable to be used in charm?
  • Do we publish ROCK regardless as long as it passed sanity tests?

I think this issue is partially trying to solve the integration testing part. On that regard, I think we should test the images before integrating with the charm. I can think of some options here:

A) we only execute sanity checks and cve scans on the rock, if those two pass, we can proceed to publish. After that, we would have to open a PR in the charm(s) repo to integrate the new image. I think this is beneficial because we avoid the overhead of having to trigger a job for running integration tests in this repo as we will leave the heavy lifting to the charm's repo (setting the testing env and run each integration test). This will also help us with the duplication problem and having to keep both repos synced.

B) We could trigger integration tests on other repos using this. It requires a bit more config, specially the part where we need to replace rocks in metadata.yaml and other files. I am not sure how to do that.

C) We clone integration tests and run them from the rocks repo, but that is not really ideal because of what you have exposed in this issue and what we have discussed.

Also, some comments about the implementation. For sanity checks, I agree we should have one test dir per rock. For integration tests (depending on which solution we implement), we should just have one integration per repo.

DnPlas commented 9 months ago

After discussing with the team, we have agreed that:

  1. This issue has outdated and irrelevant information, as there have been many changes to the rocks repositories.
  2. We will close this issue in favour of a new one where the main goal will be to use the new reusable CI for rock repositories (see here for an example).
  3. There will a child issue per repository that is still missing the CI.
orfeas-k commented 9 months ago

Here is the new issue we will use to track progress in repos #797