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

Added configuration reader #158

Merged
merged 9 commits into from May 13, 2022
Merged

Added configuration reader #158

merged 9 commits into from May 13, 2022

Conversation

teclator
Copy link
Contributor

@teclator teclator commented May 12, 2022

Problem

In #132 we added support for an initial d-installer configuration through a YAML file. This PR aims to extended it allowing the user to customize some of the default settings using the kernel command line options.

Solution

We have moved the logic for reading the configuration from the Config class to a separate class and also a class for parsing the kernel command line options has been added. DInstaller kernel options needs to be declared using the dinst prefix by now and it also allow to specify an URL for fetching the configuration from, see the example above:

dinst.config_url=http://example.org/d-installer.yaml dinst.web.ssl=MODIFIED

The reader can return a Config object merging all the configurations present in the system.

Testing

  • Added unit tests

@coveralls
Copy link

coveralls commented May 12, 2022

Pull Request Test Coverage Report for Build 2319445257

  • 108 of 116 (93.1%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.4%) to 86.19%

Changes Missing Coverage Covered Lines Changed/Added Lines %
service/lib/dinstaller/cmdline_args.rb 21 22 95.45%
service/lib/dinstaller/config.rb 26 27 96.3%
service/lib/dinstaller/config_reader.rb 59 65 90.77%
Totals Coverage Status
Change from base Build 2307922793: 0.4%
Covered Lines: 1061
Relevant Lines: 1231

💛 - Coveralls

@teclator teclator force-pushed the dinstaller_configuration branch 2 times, most recently from baeeefd to b9d87fc Compare May 12, 2022 17:31
@teclator teclator marked this pull request as ready for review May 12, 2022 18:35
@teclator teclator requested a review from imobachgs May 12, 2022 18:36
Copy link
Contributor

@dgdavid dgdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left there a few comments, but changes looks good to me. Thanks!

service/lib/dinstaller/cmdline_args.rb Outdated Show resolved Hide resolved
service/lib/dinstaller/config.rb Outdated Show resolved Hide resolved
service/lib/dinstaller/config_reader.rb Show resolved Hide resolved
service/lib/dinstaller/manager.rb Outdated Show resolved Hide resolved
service/lib/dinstaller/config.rb Show resolved Hide resolved
teclator and others added 3 commits May 13, 2022 08:55
Co-authored-by: David Díaz <1691872+dgdavid@users.noreply.github.com>
Co-authored-by: David Díaz <1691872+dgdavid@users.noreply.github.com>
Co-authored-by: David Díaz <1691872+dgdavid@users.noreply.github.com>
Copy link
Member

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a few comments, although I do not feel we should block this PR anymore. Just in case you want to have them into account for the future.

service/lib/dinstaller/config_reader.rb Outdated Show resolved Hide resolved
service/lib/dinstaller/cmdline_args.rb Outdated Show resolved Hide resolved
service/lib/dinstaller/config_reader.rb Outdated Show resolved Hide resolved
service/lib/dinstaller/config_reader.rb Outdated Show resolved Hide resolved
service/lib/dinstaller/config_reader.rb Show resolved Hide resolved
service/lib/dinstaller/cmdline_args.rb Outdated Show resolved Hide resolved
service/lib/dinstaller/config_reader.rb Show resolved Hide resolved
service/lib/dinstaller/config_reader.rb Outdated Show resolved Hide resolved
Copy link
Member

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can merge it now. Thank you!

@imobachgs imobachgs merged commit d0ac850 into master May 13, 2022
@imobachgs imobachgs deleted the dinstaller_configuration branch May 13, 2022 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants