Elektrobit / flake-pilot

Registration/Control utility for applications launched through a runtime-engine, e.g containers
MIT License
9 stars 5 forks source link

Replace adhoc yaml with serde #130

Closed Ichmed closed 1 year ago

Ichmed commented 1 year ago

first part of #128

Includes only podman config, will copy the approach for firecracker and/or create generic version for all back ends if approved

Adds lazy_static for config initialization, can be removed if rust version is bumped to one that includes OnceCell

schaefi commented 1 year ago

@Ichmed thanks for your work and effort. Your coding is great and as we discussed I expected the type of changes that are part of the pull request. On the other side there are also some changes that I wonder about and consider unrelated to the topic and that also turns the pull request into a hard to review state. To be honest I stopped reading at the commit where git told me it's too large diff to be showed by default.

I was sort of ok with the big changes regarding the clippy patch last time but going forward we should not create pull requests that are practically not in a review state. Please avoid

I also think it makes sense to create commits per responsibility. Like in an object oriented design pattern when a class is usually developed for a single responsibility also the flow of commits in a pull request can follow this style. In this particular case I though it would be useful to structure the set of commits like this

  1. Implementation of serde based reading of the yaml such that it can serve as drop in replacement of the adhoc method
  2. Changes to the code that uses adhoc yaml, now using the serde based code

That would turn things right and allows for a real review.

Let me know if this makes sense to you

Thanks

Ichmed commented 1 year ago

I didn't realize there were so many whitespace changes, I'll setup a new PR with just the content, however this means we should probably set up a rustfmt job like the clippy one to prevent this in the future.

Ichmed commented 1 year ago

Closed, will open new PR