-
Notifications
You must be signed in to change notification settings - Fork 443
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
Add Github Actions CI for charm operators #1407
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this contribution @knkski!
I left comments.
.github/workflows/test-microk8s.yaml
Outdated
run: | | ||
set -eux | ||
./scripts/v1beta1/build.sh -r docker.io/kubeflowkatib -t latest | ||
for image in katib-ui katib-controller katib-db-manager; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to update other images also?
For example: docker.io/kubeflowkatib/file-metrics-collector
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually took out the usage of ./scripts/v1beta/build.sh
, as building all of those images took up too much disk space.
I was thinking about only building the images as they get included in testing, and right now the only test is a simply connectivity test. I'm going to be working on expanding that, but wanted to keep it simple initially.
.github/workflows/test-microk8s.yaml
Outdated
@@ -0,0 +1,83 @@ | |||
name: Charmed Katib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you name this file test-microk8s.yml
?
I don't see that this action is running on this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the name of the file to match this name. As far as why it's not showing up, do you have to enable actions in settings or anything? I'm not sure exactly why it's not showing up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Bobgy Can you help us with enabling GitHub actions on the Katib repository, please ?
I think it should be pretty straightforward: https://docs.github.com/en/free-pro-team@latest/github/administering-a-repository/disabling-or-limiting-github-actions-for-a-repository#managing-github-actions-permissions-for-your-repository
/cc @gaocegege @johnugeorge
.github/workflows/test-microk8s.yaml
Outdated
- name: Install dependencies | ||
run: | | ||
set -eux | ||
sudo snap install microk8s --classic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you are not using any pre-existing GitHub actions (e.g. uses: actions/checkout@v2
) to install microk8s, does GitHub actions have any advantages compare to Travis ?
What do you think @gaocegege @johnugeorge @knkski ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/helm/kind-action We can use this action, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated it to use the balchua/microk8s-actions@v0.2.1
action. I have a slight preference for github actions due to the UI being better about scrolling through large amounts of output, but can switch to Travis CI if desired.
ping @andreyvelich /lgtm |
/hold |
Builds images locally with `latest` tag, and includes new bundle for testing purposes that sets operator docker images to `latest` as well instead of a particular revision.
Pushed up an update that bumped the version of |
The page doesn't load atm, I'll take a look later. |
I don't think I need to change anything.
so is katib repo |
Thank you for helping with this! |
@knkski In the meantime, can you try to create PR on your local |
Created knkski#2 to test in the meantime |
Thanks @yuzisun, it's strange why KFServing and Katib have various GitHub actions policies. |
Yes, it shows all actions are enabled |
I believe GitHub actions should be activated once we merge this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreyvelich, knkski The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Adds Github Actions CI for charm operators, to ensure that there's no breakages / regressions
Which issue(s) this PR fixes:
N/A
Special notes for your reviewer:
Adds new bundle for CI purposes that points at the
latest
tags instead of particular revisions.Release note: