-
Notifications
You must be signed in to change notification settings - Fork 7
feat(ci): dependabot configuration + using helm/kind-action for e2e cluster setup #49
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
version: 2 | ||
updates: | ||
- package-ecosystem: "gomod" | ||
directory: "/" | ||
schedule: | ||
interval: "weekly" | ||
open-pull-requests-limit: 10 | ||
groups: | ||
go-modules: | ||
patterns: | ||
- "*" | ||
pull-request-title: | ||
prefix: "chore(deps)" | ||
separator: " " | ||
labels: | ||
- "dependencies" | ||
- "go" | ||
|
||
- package-ecosystem: "github-actions" | ||
directory: "/" | ||
schedule: | ||
interval: "weekly" | ||
open-pull-requests-limit: 10 | ||
groups: | ||
actions-updates: | ||
patterns: | ||
- "*" | ||
update-types: | ||
- "patch" | ||
- "minor" | ||
- "major" | ||
pull-request-title: | ||
prefix: "chore(deps-dev)" | ||
separator: " " | ||
labels: | ||
- "dependencies" | ||
- "github-actions" |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,23 +1,35 @@ | ||||||
name: Lint | ||||||
name: Linter | ||||||
|
||||||
on: | ||||||
push: | ||||||
branches: [ "main" ] | ||||||
pull_request: | ||||||
branches: [ "main" ] | ||||||
|
||||||
permissions: | ||||||
contents: read | ||||||
|
||||||
jobs: | ||||||
lint: | ||||||
name: Run on Ubuntu | ||||||
name: Run Linting | ||||||
runs-on: ubuntu-latest | ||||||
|
||||||
steps: | ||||||
- name: Clone the code | ||||||
- name: Checkout code | ||||||
uses: actions/checkout@v4 | ||||||
|
||||||
- name: Setup Go | ||||||
uses: actions/setup-go@v5 | ||||||
with: | ||||||
go-version-file: go.mod | ||||||
cache: true | ||||||
|
||||||
- name: Go mod tidy | ||||||
run: go mod tidy | ||||||
|
- name: Go mod tidy | |
run: go mod tidy |
Other context: #42 (comment)
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.
#44 will introduce a workflow to enforce that, if you get time please take a look
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.
Yes, I decided to still add it as I figured the purpose of the workflow is to just run a linter, not check if it is up to date or not, which I believe Kevin's PR on the new gh workflow would check by itself
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.
From my point of view, it makes little sense to add a test to check whether the go.mod
file is up-to-date like on #44 and update the go.mod
file on other workflows.
What would be the goal? Running tests on a state that will not be merged due to the fact its not compilable? From my point of view this would burn valuable CI time of the Sigstore organisation.
Maybe I am missing something here, but for me it's really hard to see the value with this approach :)
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.
Which is exactly why I removed it from all workflows assuming #44 will be merged, hence never running actions on a state of the project that will not actually be merged
I left a comment at the bottom about this too.
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.
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -2,50 +2,39 @@ name: E2E Tests | |||
|
||||
on: | ||||
push: | ||||
branches: [ "main" ] | ||||
pull_request: | ||||
branches: [ "main" ] | ||||
|
||||
permissions: | ||||
contents: read | ||||
|
||||
jobs: | ||||
test-e2e: | ||||
name: Run on Ubuntu | ||||
name: Run E2E Tests | ||||
runs-on: ubuntu-latest | ||||
steps: | ||||
- name: Clone the code | ||||
- name: Checkout code | ||||
uses: actions/checkout@v4 | ||||
|
||||
- name: Setup Go | ||||
uses: actions/setup-go@v5 | ||||
with: | ||||
go-version-file: go.mod | ||||
|
||||
- name: Install the latest version of kind | ||||
run: | | ||||
curl -Lo ./kind https://kind.sigs.k8s.io/dl/latest/kind-linux-amd64 | ||||
chmod +x ./kind | ||||
sudo mv ./kind /usr/local/bin/kind | ||||
|
||||
- name: Verify kind installation | ||||
run: kind version | ||||
|
||||
- name: Install kubectl | ||||
run: | | ||||
curl -LO "https://dl.k8s.io/release/$(curl -L -s https://dl.k8s.io/release/stable.txt)/bin/linux/amd64/kubectl" | ||||
chmod +x kubectl | ||||
sudo mv kubectl /usr/local/bin/kubectl | ||||
|
||||
- name: Verify kubectl installation | ||||
run: kubectl version --client | ||||
cache: true | ||||
|
||||
- name: Create kind cluster | ||||
run: kind create cluster | ||||
uses: helm/kind-action@v1 | ||||
with: | ||||
cluster_name: kind | ||||
Comment on lines
+27
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That looks like a great improvement. But its only usable on the github workflow. To make it easier to run our e2e tests on local machines or other CI pipelines does it make sense to provide the general as Here we would then just need to run: make tools kind-start Where A .PHONY: kind-start
kind-start: kind
$(KIND) create cluster --name model-validation-testing --config kind-1-33-0.yaml
.PHONY: tools
tools:
$(call go-get-tool,$(KIND),sigs.k8s.io/kind,$(KIND_VERSION)) It would make it much easier to reproduce CI issues locally. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note this doesn't change the existing behaviour, it replaces the SDK scaffolded bits with the helm kind action to install/run the cluster. I think this is cleaner than what was there previously. The makefile already has options for running the e2e test against an existing kind cluster, just not for creating it. I also find that act is good for running workflows locally, if we need to reproduce CI issues. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ^ Agreed, we can include in the docs how to run workflows locally soon instead |
||||
|
||||
- name: Wait for cluster to be ready | ||||
- name: Wait for kind cluster to be ready | ||||
run: | | ||||
echo "Waiting for cluster to be ready..." | ||||
kubectl wait --for=condition=Ready nodes --all --timeout=300s | ||||
kubectl wait --for=condition=Ready --namespace=kube-system pod --all --timeout=300s | ||||
echo "Cluster is ready" | ||||
|
||||
- name: Running Test e2e | ||||
- name: Run E2E tests | ||||
run: | | ||||
go mod tidy | ||||
|
go mod tidy |
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.
FYI That's left over from a previous PR, scaffolded by the SDK.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,9 @@ name: Tests | |
|
||
on: | ||
push: | ||
branches: [ "main" ] | ||
pull_request: | ||
branches: [ "main" ] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we can then get rid of this entire workflow? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really want to remove this? I think we should keep unit tests and e2e tests There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, we need to keep, this workflow is responsible for running the general unit tests, then the other workflow is specifically for the e2e tests we have so far |
||
|
||
jobs: | ||
test: | ||
|
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.
Just for clarification this will just run the CI whenever a PR is opened against the main branch, right?
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.
yes, that's right. It was previously push/PRs based on any branch