feat(ci): dependabot configuration + using helm/kind-action for e2e cluster setup #49
Conversation
|
This needs renaming, since no E2E tests were added. The only new addition seems to be the dependabot addition, other than that it just appears to be reworking what's already there and restricting some things to just running on main related pushes/PRs. Am I missing something? |
Yes apologies, I meant it as in adding e2e actions configuration but I can see how it can be misunderstood so I will change it |
SequeI
changed the title
…luster setup Signed-off-by: SequeI <[email protected]>
SequeI
changed the title
| push: | ||
| branches: [ "main" ] | ||
| pull_request: | ||
| branches: [ "main" ] |
There was a problem hiding this comment.
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.
yes, that's right. It was previously push/PRs based on any branch
.github/workflows/lint.yml
Outdated
| - name: Go mod tidy | ||
| run: go mod tidy |
There was a problem hiding this comment.
Seeing this I would still advocate for good practice in failing the build if the code checked into the repository is not up-to-date and can not be compiled.
| - name: Go mod tidy | |
| run: go mod tidy |
Other context: #42 (comment)
There was a problem hiding this comment.
#44 will introduce a workflow to enforce that, if you get time please take a look
There was a problem hiding this comment.
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.
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.
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.
.github/workflows/test-e2e.yml
Outdated
| - name: Running Test e2e | ||
| - name: Run E2E tests | ||
| run: | | ||
| go mod tidy |
There was a problem hiding this comment.
FYI That's left over from a previous PR, scaffolded by the SDK.
| uses: helm/kind-action@v1 | ||
| with: | ||
| cluster_name: kind |
There was a problem hiding this comment.
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 Makefile rules?
Here we would then just need to run:
make tools kind-startWhere tools would install kind and whatever other tools we need for development into ${PWD}/bin/tools on this repository. We can pin the kind version in the Makefile.
A kind-start rule could look like this:
.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.
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.
^ Agreed, we can include in the docs how to run workflows locally soon instead
| push: | ||
| branches: [ "main" ] | ||
| pull_request: | ||
| branches: [ "main" ] |
There was a problem hiding this comment.
I guess we can then get rid of this entire workflow?
There was a problem hiding this comment.
Do we really want to remove this? I think we should keep unit tests and e2e tests
There was a problem hiding this comment.
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
Signed-off-by: SequeI <[email protected]>
|
I've removed go mod tidy where not needed, my thinking is that if we will have a workflow to specifically check if go mod tidy causes differences and fails if there are, it is probably not needed in workflows and we can just assume it is fine unless as I said that workflow fails |
3 participants
Summary
Release Note
Documentation