Automated Clippy Action

[Ichmed]

• Added a Clippy action to Github.
• Fixed all Clippy warnings and errors except for function argument numbers
• Fixed up some (but not all) unidiomatic code

The Action will run for every push for now and fail if any Clippy warnings are generated. There is a nicer vrsion that annotates the code with the warnings directly but I could not get it to run https://github.com/actions-rs/clippy-check

All code should be semantically identical except for flake-ctl/src/app_config.rs#134 which now removes ALL leading slashes, not just one, could be a problem but I don't know enough about the app yet.

[schaefi]

@Ichmed thanks for your effort on consolidating/simplifying the code and the clippy integration. This is very much appreciated.

On the other hand I'm afraid this is a huge pull request, it fails to build now and it introduces large diffs that are very hard to review. To be honest I cannot tell if it will not introduce breakage to the functionality at some point.

So can we go in smaller steps, small code changes, easy to review, easy to test and and on its own not introducing potential regressions ?

@isbm are you involved in the cleanup tasks here ? It's really good and important to do this, but not in one big chunk. Can we find an agreement here how to slice the changes without harming what @Ichmed has already done ?

Thanks much

[Ichmed]

Me and @isbm talked about it and thought the best idea was to fix all in one go so the action doesn't fail after the merge, but of course we can break it down and add the action at the end

[Ichmed]

Runs green now, forgot to add some of the sub-crates to the workspace

[schaefi]

Me and @isbm talked about it and thought the best idea was to fix all in one go so the action doesn't fail after the merge, but of course we can break it down and add the action at the end

If it stays a big change I'd like to build a new package with the patch applied and do some tests. If all continues to work I would be ok to merge a big blob. I have some use cases on my side which would be very unfortunate if they fail in the phase of promoting the tooling. Thus please give me some time for testing. Thanks

[isbm]

If it stays a big change I'd like to build a new package with the patch applied and do some tests. If all continues to work I would be ok to merge a big blob. I have some use cases on my side which would be very unfortunate if they fail in the phase of promoting the tooling. Thus please give me some time for testing. Thanks

You've got it. 😉

Also: if you find something, that would also mean lack of unit test, basically. So please report back, we then will go step-by-step, introducing more tests instead.

My point is: we would like to get rid of lack of linter early and put hi-prio fixing in case we break something. Git remembers everything so we can quickly restore broken stuff. But we would also like to see Clippy check ASAP and see it green so every further PRs will not introduce us more and more work to keep fixing it.

[schaefi]

@m-kat you have approved these changes, did you test all of flake-pilot for functionality ? Meaning

• [x] podman-pilot, pull, registration, run of container, delta container, with resume
• [x] firecracker-pilot, pull, registration, run of VM from the firecracker CI space, run of VM we build ourselves, run with overlay, run without overlay, with resume (vsock)
• [x] firecracker-pilot networking, test procedure as documented

Did someone really run all these tests prior clicking on the approve button ? I was not so fast to be honest and still checking. Please no merge until the above can be checked off. Thanks

[isbm]

@m-kat you have approved these changes

Technically, I approved from the POV that I would even be OK with regressions in master branch, so we could after-fix them. Maybe I should have been clicked only on "Comment" button, you are right.

Please no merge until the above can be checked off.

Like I said: you've got it. No merge until you agree.

[schaefi]

Finished my tests, looks good. @Ichmed thanks much you did several good changes to the code to make it a better read, much appreciated :+1:

[schaefi]

I found a little glitch and fixed by direct master push b0bf478753cd5f2804cf8e6ce66f53b1d1380ca6

sorry I have a presentation on the topic on Monday and I need the stuff to work, thus the direct push to master