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

[dist] Fix initial presets handling in obsstoragesetup. #12628

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ionic
Copy link

@Ionic Ionic commented May 25, 2022

The general idea is that, if a user provided a preset using
/etc/buildhost.config, i.e., if this file exists at the start of the
(first) script run, then it should be moved to
/etc/buildhost.config.presets, assuming that this file does not exist
already (which is meant as a protection for later runs).

Unfortunately, the logic for doing so was broken for 12 years and nobody
noticed it.

During the first run, the current code:

  • checks if buildhost.config.presets exists, and if it DOES NOT:
  • moves buildhost.config to buildhost.config.presets
  • creates buildhost.config from scratch
  • appends buildhost.config.presets to buildhost.config (if existent)

This has the unwanted side-effect that if no buildhost.config file has
been provided by the user, nothing will be moved in the first run and
the second run will see a buildhost.config file, but no
buildhost.config.presets file, so it wrongly assumes that the
buildhost.config is a user-provided preset and moves it to
buildhost.config.presets.

Due to that, the initial, default config will essentially always
override any user-defined value in /etc/sysconfig/obs-server or even
when modifying /etc/buildhost.config directly.

The fix for that is trivial: if neither files exist, create
buildhost.conf.presets as an empty file.

While that will fix the issue for any NEW installations, older ones will
not be affected. Sadly, it's not possible to fix existing installations,
since we lost the information regarding user-provided or auto-generated
config entries long ago due to the initial bug.

See: #12520

@hennevogel hennevogel added the Appliance 💿 Things regarding our official appliances label May 25, 2022
The general idea is that, if a user provided a preset using
/etc/buildhost.config, i.e., if this file exists at the start of the
(first) script run, then it should be moved to
/etc/buildhost.config.presets, assuming that this file does not exist
already (which is meant as a protection for later runs).

Unfortunately, the logic for doing so was broken for 12 years and nobody
noticed it.

During the first run, the current code:
  - checks if buildhost.config.presets exists, and if it DOES NOT:
  - moves buildhost.config to buildhost.config.presets
  - creates buildhost.config from scratch
  - appends buildhost.config.presets to buildhost.config (if existent)

This has the unwanted side-effect that if no buildhost.config file has
been provided by the user, nothing will be moved in the first run and
the second run will see a buildhost.config file, but no
buildhost.config.presets file, so it wrongly assumes that the
buildhost.config is a user-provided preset and moves it to
buildhost.config.presets.

Due to that, the initial, default config will essentially always
override any user-defined value in /etc/sysconfig/obs-server or even
when modifying /etc/buildhost.config directly.

The fix for that is trivial: if neither files exist, create
buildhost.conf.presets as an empty file.

While that will fix the issue for any NEW installations, older ones will
not be affected. Sadly, it's not possible to fix existing installations,
since we lost the information regarding user-provided or auto-generated
config entries long ago due to the initial bug.

See: openSUSE#12520
@Ionic Ionic force-pushed the bugfix/obsstoragesetup-initialize-presets-if-null branch from 095e419 to bae8397 Compare April 17, 2023 15:31
@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

Merging #12628 (095e419) into master (2cfbbd4) will increase coverage by 0.76%.
Report is 1064 commits behind head on master.
The diff coverage is n/a.

❗ Current head 095e419 differs from pull request most recent head bae8397. Consider uploading reports for the commit bae8397 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12628      +/-   ##
==========================================
+ Coverage   88.12%   88.89%   +0.76%     
==========================================
  Files         734      684      -50     
  Lines       24637    23431    -1206     
==========================================
- Hits        21712    20829     -883     
+ Misses       2925     2602     -323     

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Appliance 💿 Things regarding our official appliances
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants