Skip to content
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

Pass along containerd config as return value #4160

Merged
merged 5 commits into from
Mar 28, 2024

Conversation

twz123
Copy link
Member

@twz123 twz123 commented Mar 13, 2024

Description

  • Pass along containerd config as return value
    Return the containerd config as a string instead of having the configurer write it to disk. This reduces the number of struct fields required by the configurer, and makes testing less dependent on the file system.

    Let the containerd component write the file instead.

    Remove the hard-coded string literal for the paths to containerd-cri.toml and use k0s' RunDir instead. This already takes into account the differences between Windows and UNIX-like operating systems, and also takes into account the different defaults when running as non-root user.

    Remove the escapedPath function and inline it directly before the containerd component generates the template.

  • Use toml.Marshal to generate default CRI config
    This is more convenient and removes the necessity to declare some one-shot writer variable to get the marshalled data. Moreover, make this a free-standing function, so it's more obvious that the default config is solely dependent on the pause image.

  • Simplify hasCRIPluginConfig

    • Use toml.Tree's HasPath function to simplify the check for the CRI plugin config section
    • Make it a free-standing function. The only reason why it had the receiver was to use the usage of the logger.
    • Remove unit tests for it, as all the interesting cases are already covered by the unit tests for handleImports and don't add anything of interest.
    • Add some more comments, refine log statements and error messages.
  • Refactor CRI config tests

    • Merge two nearly identical configurer tests
      The two tests "should merge CRI configs" and "should have single import for all CRI configs" were nearly doing the same. One of them was missing two assertions. Combine them into one test.
    • Rename srvconfig import alias to the more idiomatic serverconfig
    • Adjust variable names to more accurately reflect their purpose
    • Transition from require to assert in certain test assertions to continue execution after a failure, allowing for more comprehensive test feedback.
    • Use upper-case CRI instead lower-case cri in descriptive strings

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

  • Manual test
  • Auto test added

Checklist:

  • My code follows the style guidelines of this project
  • My commit messages are signed-off
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

twz123 added 5 commits March 27, 2024 17:33
This is more convenient and removes the necessity to declare some
one-shot writer variable to get the marshalled data. Moreover, make this
a free-standing function, so it's more obvious that the default config
is solely dependent on the pause image.

Signed-off-by: Tom Wieczorek <[email protected]>
* Use toml.Tree's HasPath function to simplify the check for the CRI
  plugin config section
* Make it a free-standing function. The only reason why it had the
  receiver was to use the usage of the logger.
* Remove unit tests for it, as all the interesting cases are already
  covered by the unit tests for handleImports and don't add anything
  of interest.
* Add some more comments, refine log statements and error messages.

Signed-off-by: Tom Wieczorek <[email protected]>
The two tests "should merge CRI configs" and "should have single import
for all CRI configs" were nearly doing the same. One of them was missing
two assertions. Combine them into one test.

Signed-off-by: Tom Wieczorek <[email protected]>
* Rename `srvconfig` import alias to the more idiomatic `serverconfig`
* Adjust variable names to more accurately reflect their purpose
* Transition from `require` to `assert` in certain test assertions to
  continue execution after a failure, allowing for more comprehensive
  test feedback.
* Use upper-case CRI instead lower-case cri in descriptive strings

Signed-off-by: Tom Wieczorek <[email protected]>
Return the containerd config as a string instead of having the
configurer write it to disk. This reduces the number of struct fields
required by the configurer, and makes testing less dependent on the
file system.

Let the containerd component write the file instead.

Remove the hard-coded string literal for the paths to
containerd-cri.toml and use k0s' RunDir instead. This already takes into
account the differences between Windows and UNIX-like operating systems,
and also takes into account the different defaults when running as
non-root user.

Remove the escapedPath function and inline it directly before the
containerd component generates the template.

Signed-off-by: Tom Wieczorek <[email protected]>
@twz123 twz123 force-pushed the return-containerd-config branch from f18840b to cec9435 Compare March 27, 2024 16:33
@twz123 twz123 marked this pull request as ready for review March 27, 2024 18:58
@twz123 twz123 requested a review from a team as a code owner March 27, 2024 18:58
@twz123 twz123 requested review from kke and jnummelin March 27, 2024 18:58
@twz123 twz123 merged commit c4cf70d into k0sproject:main Mar 28, 2024
77 checks passed
@twz123 twz123 deleted the return-containerd-config branch March 28, 2024 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants