Increase CI coverage and provide new dev tool #982
Conversation
|
Please do not squash merge this PR. FF or rebase merge would be ideal. |
|
|
evrardjp
force-pushed
the
remove_flags
branch
2 times, most recently
from
e7e3de0
to
8fc526d
Compare
|
Bonus feature added on tests: Testing that cordoning before the reboot stays applied. |
|
Failure due to rate limiting of trivy :/ |
evrardjp
force-pushed
the
remove_flags
branch
2 times, most recently
from
4c11090
to
867ff5f
Compare
|
Updated the CI matrix to make it a bit simpler. |
evrardjp
force-pushed
the
remove_flags
branch
from
e816cd9
to
a7219c6
Compare
|
fixed conflicts due to recent merges of .github worklows. |
|
Overall this looks good. The safest way to land this would probably be to cleanup the CI first as a separate PR (and then potentially iterate a bit if we discover gremlins), and then when we are very confident in the health of our CI and tests, we can land the refactor, which should just pass the E2E tests (my first read of the code suggests that there are no [intended] functional changes to the behavior, just code optimization). Great work, this will definitely be way easier to maintain! :) |
Without this, we have to rename files at every version. This is really unnecessary, we should only change the files and be done with it. This is a problem, as if we move to programmatic test running, the tests would need to be mutatated at every k8s version. With this model, we know that only the kind-cluster files need to be modified for the tests to ba automatically adapted. Signed-off-by: Jean-Philippe Evrard <open-source@a.spamming.party>
|
There is a small hiccup with the approach of adding CI first, is that right now we have bugs, and the CI caught it ;) My other big PR is fixing some of those. However, I am fine to have this MR being just the CI: We would merge with some tests of CI broken, and we know the next PRs will fix it. This way it makes it very clear of the intent. |
evrardjp
changed the title
evrardjp
force-pushed
the
remove_flags
branch
from
a7219c6
to
09a17ad
Compare
Without this, e2e tests need tons of manual work to test locally, and the results are not easily exposed. People are less likely to use the e2e tests if they are tough to use outside the CI. This commit makes it easier to run tests locally, and ensures the CI is closer to the Makefile. At the same time, this removes debt in the github worfklows: By switching to newer versions of kind, we can remove the very old workaround for the failed to attach pid 1. Signed-off-by: Jean-Philippe Evrard <open-source@a.spamming.party>
Without this, impossible to prove that the node stays as cordonned after a reboot by kured. This refactor also adds the test in the CI, and makes sure the CI is a bit simpler, by using matrix more extensively. Signed-off-by: Jean-Philippe Evrard <open-source@a.spamming.party>
evrardjp
force-pushed
the
remove_flags
branch
2 times, most recently
from
368d675
to
f4ce6d8
Compare
Makefile
Outdated
| echo "Running short go tests" | ||
| go test -test.short -json ./... > test.json | ||
| echo "Running shellcheck" | ||
| find . -name '*.sh' -exec .tmp/shellcheck {} \; |
There was a problem hiding this comment.
It doesn't seem to fail either: https://github.com/kubereboot/kured/actions/runs/11305350571/job/31444749412?pr=982#step:4:116
There was a problem hiding this comment.
I am not a Makefile expert, either this behaviour was already present or we should remove find. Any idea? Proposition?
| @@ -1,23 +1,25 @@ | |||
| .DEFAULT: all | |||
| .PHONY: all clean image minikube-publish manifest test kured-all | |||
|
|
|||
| TEMPDIR=./.tmp | |||
There was a problem hiding this comment.
.tmp seems to be still used in other places.
There was a problem hiding this comment.
Damn I shouldn't have changed that in last minute :)
There was a problem hiding this comment.
I didn't find it in another places. The grep didn't return any result. What else did I miss? Are you sure you're looking at the last commit? Does it block this improvement?
evrardjp
force-pushed
the
remove_flags
branch
from
f4ce6d8
to
efea495
Compare
|
@dholbach I removed the .tmp in the find and moved to xargs instead of exec. Seems it's doing the trick. |
This is more idiomatic. Signed-off-by: Jean-Philippe Evrard <open-source@a.spamming.party>
evrardjp
force-pushed
the
remove_flags
branch
from
efea495
to
dab11fe
Compare
|
@evrardjp this looks like just CI changes now, correct? /lgtm |
|
@jackfrancis can you approve then? |
|
Sadly this was merged into a single commit, so I hope we won't need a partial revert. I will keep the branch remove_flags temporarily, in case we need a revert. |
Without this, it is hard to adapt the code and test it locally.
This MR contain multiple changes, separately mergeable. In order to faster iterate, I made a single PR.
With this MR, we can more easily test locally kured, and the CI is closer to the local tests.
The next step, after this merges, is to bump golang, kube versions, and continue cleaning up the code.
This will open the way for the following PRs: