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

ci/cd: simplify and refactor workflows #246

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

d3adb5
Copy link
Collaborator

@d3adb5 d3adb5 commented Apr 2, 2023

We discussed switching from running a dry-run install on the Single Node OpenShift clusters as self-hosted runners and instead using kind or k0s.

This PR changes the CI and CD workflows to use kind for a dry-run. The CI workflow is now invoked from the CD workflow to avoid repetition, and Tilt has been removed altogether since I couldn't see why the Tilt setup was necessary.

Bringing back the Tilt files should be as easy as running git revert on 645165f. If we end up having to use it again, that's how we can restore them.

Additionally, linting, unit testing, and the dry run install happen in parallel now. Only the latter needs a cluster, so that's where we bring up kind. For now, since we're not testing specific cluster topologies, no cluster configuration has been added.

Simplify the continuous integration workflow by renaming it and making
most of the jobs run in parallel. Additionally, the workflow runs on a
GitHub hosted runner instead of the Single Node OpenShift runner.
Simplify the CD workflow by invoking a CI build instead of reproducing
CI steps on it, as well as splitting the remaining part of the workflow
into multiple jobs.
Remove the Tiltfile and Tiltfile-delete files, since Tilt is no longer
executed in CI. This is being done in a separate commit so that should
Tilt be re-introduced, the files can be restored with a simple revert.
Add an input to the CI workflow to allow skipping sending a Slack
notification, as it might not be necessary.
@d3adb5
Copy link
Collaborator Author

d3adb5 commented Apr 2, 2023

It seems every time I read over the documentation and reference for GitHub Actions I find something has changed. I'd love to get some input from everybody on these changes. @rasheedamir @aslafy-z @slauger Can you guys leave a review?

The updated actions won't run here until merged, so I tested them on my fork:

And in the #testing-pr-notifications channel on Stakater's Slack community.

@d3adb5
Copy link
Collaborator Author

d3adb5 commented Apr 2, 2023

About the failed status check: the Tilt files were removed in this PR, so tilt ci will definitely fail. I'm not sure how to get GitHub Actions to run the updated workflows here. 😕

@rasheedamir
Copy link
Member

Lets do it incrementally; we keep the current workflows as is and add new ones and when the new one has all features of the existing workflow then we can remove

We want to test and validate everything i.e. route; grafana dashboard; etc.

We use tilt to setup stuff on the SNO

@rasheedamir
Copy link
Member

also we have a plan that we should add real integration tests as well i.e. we deploy one sample application and then validate all of the resources are created and are valid with tests; so, we can guarantee 100% quality of this chart

@d3adb5
Copy link
Collaborator Author

d3adb5 commented Apr 2, 2023

We want to test and validate everything i.e. route; grafana dashboard; etc.

I see! Does Tilt test those automatically when we run tilt ci? I'm pretty unfamiliar with Tilt, so it wasn't clear to me by looking at the current workflows.

@mustafaStakater
Copy link
Contributor

mustafaStakater commented Apr 3, 2023

helm install dry run should be run with values-test.yaml because it deploys more resources. you will need to install few helm charts for sealedsecrets, grafana, imc etc in kind before proceeding with helm install

@aslafy-z aslafy-z self-requested a review July 6, 2023 22:59
@mustafaStakater
Copy link
Contributor

how do we plan on making the missing crds available in kind?

Copy link
Member

@rasheedamir rasheedamir left a comment

Choose a reason for hiding this comment

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

Missing CRDs in kind how do we resolve it? Also the goal is that we are testing on real cluster so, can guarantee the quality of the chart

@karl-johan-grahn
Copy link
Contributor

Talked about it at Open Source pruning session today:

  • Review the need for this change
  • Feasibility of installing all CRDs on a temporary cluster

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants