cobbler / cobbler-tftp

Experimental - Cobbler stateless TFTP server
GNU General Public License v2.0
4 stars 2 forks source link

Add parsing of settings parameters #12

Closed ByteOtter closed 1 year ago

ByteOtter commented 1 year ago

Linked Items

Fixes #7

## Description Add a settings module which will contain the default `settings.yml` file aswell as the logic for collecting all application settings from all the different sources. You can track progress in the chart below |Source|Functionality|Status| |-|-|-| |`settings.yml`|Takes the path to a configuration file. If none is given the standard `settings.yml` file is used.|:heavy_check_mark:| |`environment varaibles`|Get collected after `settings.yml`. If set their keys will replace the keys in the settings dict taken from the `yml` file|:heavy_check_mark:| |`cli-arguments`|Get collected from `click` CLI after everything else. If set they will overwrite or add to the keys already present in the dict.|:heavy_check_mark:| All of these sources will be aggregated into a single `dictionary` within the `settings_factory` class.
This `dict` will then be validated by a `validate.yml` separate function and handed to the factory's `build()` function which, if the return value is `true`, will build a `Settings` object ## Behaviour changes New: Cobbler-TFTP can now be configured by a config file, environment variables or cli parameters ## Category This is related to a: - [ ] Bugfix - [x] Feature - [ ] Packaging - [ ] Docs - [ ] Code Quality - [ ] Refactoring - [ ] Miscellaneous ## Tests - [x] Unit-Tests were created - [ ] System-Tests were created - [ ] Code is already covered by Unit-Tests - [ ] Code is already covered by System-Tests - [ ] No tests required
SchoolGuy commented 1 year ago

This is a feature of the application. You will write a changelog for this one. :)

ByteOtter commented 1 year ago

This is a feature of the application. You will write a changelog for this one. :)

Yep I figured. I will put this back for now and write it when this feature is done :)

Edit: Just noticed I added that label there, sorry about that

SchoolGuy commented 1 year ago

@ByteOtter Since you are adding the first tests, it is time to add a workflow for executing those added tests.

ByteOtter commented 1 year ago

@ByteOtter Since you are adding the first tests, it is time to add a workflow for executing those added tests.

Will do. Though apparently for my tests to work I must implement the validation function first so I will do that, then create a workflow.

ByteOtter commented 1 year ago

@SchoolGuy I just pushed the first draft of the validate function. There are some points I would like some input:

  1. In the default settings.yaml file the first key is schema:1. I assume from this that there could be more than one schema or other configuration files that should not serve as schemas (The user would presumably have to set schema:0) Should there be a check for this or am I wrong on this assumption?
  2. Theoretically the path of the config file cobbler-tftp should check could change. Should there also be a path given to the function?
  3. When you take a look at the final try-catch block I find it ... ugly, for lack of a better term, to return False in case of an exception. I thought about forgoing the return type of the function altogether and just have it raise an exception or not. Question would be how I check "If no exception: build Settings object"

EDIT: I was just informed that I will need a sperate settings_schema file to compare to as validate apparently needs information about the datatype it should expect. I will fix this tomorrow.

SchoolGuy commented 1 year ago

@ByteOtter We can have a chat about these questions via Slack today for sure. Since you asked here I will summarize a rough answer to your questions already:

  1. schema: 1 is just an indicator of which settings schema version we want to use. This means that we will have a Python sub-module that is named schema which contains just the schemas of the application. Your validate function will then dynamically load these modules depending on the default schema version for the application version and/or the key in the settings file.
  2. Validate is not concerned with filesystem and/or other config sources. It will just check if the blended dictionary has the right schema for the version implied by schema: 1.
  3. This part of the implementation is not correct at the moment. We will discuss the why in our Slack Huddle but in short the problem is that you need to pre-define the schema inside Python. No try-except needed.
ByteOtter commented 1 year ago

Thank you for clarifying some things today. I just pushed the basics for what I think I will need to validate the configuration created by the settings_factory.
From what I can tell these steps are still missing:

Keyword Description Status
Versioning All schemas will be located in a vx_x.py file together with their "helper" functions like validate, normalize and migrate. From what I understood the file used depends on the value for the schema key in the settings dictionary and will be selected automatically.
This logic is still missing.
:heavy_check_mark:
Conditional Settings Build Depending on the result of the validate_config method for a given schema, the settings_factory should build a Settings-Object (or not if it doesn't validate).
This logic is still missing.
:heavy_check_mark:

Correct me if I misunderstood something.
These to me seem the next logical steps to take. (except for the migrate function which I will postpone for now.)

SchoolGuy commented 1 year ago

@ByteOtter and I cleaned up the Git history to improve the reviewability of the Pull Request.

ByteOtter commented 1 year ago

@ByteOtter and I cleaned up the Git history to improve the reviewability of the Pull Request.

Thank you!

SchoolGuy commented 1 year ago

@ByteOtter Sadly I have two mention two things:

  1. You didn't keep the commit history clean. I would wish that this is now kept since the PR is coming into a state where we could merge with a clean history.
  2. Codacy has reported various issues that are at least in part correct. As such please take a look and correct them if reasonable. If it is not reasonable please mention the why in a comment here.
SchoolGuy commented 1 year ago

Codacy can't properly handle force pushes it seems. As such a second run of the analyze is in order. I will fixup the remaining issues and after the PR is green @ByteOtter has the honor of merging it.

SchoolGuy commented 1 year ago

I thought the whitelist file was enough to add. Maybe Codacy didn't get the right commit... The complexity we can't get down since this is one of the initial features.