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

Workflows for ROCK repositories #692

Closed
i-chvets opened this issue Sep 6, 2023 · 5 comments
Closed

Workflows for ROCK repositories #692

i-chvets opened this issue Sep 6, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@i-chvets
Copy link
Contributor

i-chvets commented Sep 6, 2023

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:

  • Checkout corresponding charms repository.
  • Update required files to refer to locallyt built ROCK container image (eg. metadata.yaml for charm managed workloads, config maps and/or other files for others.)
  • Execute charm's integration tests.

Pull request automation

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

  • Build ROCK using Rockcraft action.
  • Execute unit tests (smoke tests) specified in test/ directory for that ROCK.
  • Execute integration tests (if available for the ROCK) using corresponding charms integration tests. ?????
  • Scan ROCK for CVEs and generate report using common CVE scanning CI.

Merge automation

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

  • Merge pull request in to main branch.
  • Using Charmed Kubeflow registry setup in Github variables publish ROCK container image to Charmed Kubeflow container registry (https://hub.docker.com/u/charmedkubeflow)

List of repositories:

@i-chvets i-chvets changed the title Workflows for ROCK repositories (WIP) Workflows for ROCK repositories Sep 11, 2023
@i-chvets i-chvets added the enhancement New feature or request label Sep 11, 2023
@i-chvets
Copy link
Contributor Author

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
Copy link
Contributor Author

@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?

@DnPlas
Copy link
Contributor

DnPlas commented Sep 25, 2023

@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
Copy link
Contributor

DnPlas commented Jan 18, 2024

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.

@DnPlas DnPlas closed this as completed Jan 18, 2024
MLOps Solution Issues automation moved this from Needs discussion to Done Jan 18, 2024
@orfeas-k
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

3 participants