Katello / katello-packaging

[DEPRECATED] Packaging for Katello
7 stars 33 forks source link

Fixes #19481 - replace yaml files that are not answer files #439

Closed Klaas- closed 7 years ago

theforeman-bot commented 7 years ago

@Klaas-, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).


This message was auto-generated by Foreman's prprocessor

Klaas- commented 7 years ago

I'd say the ticket is in the correct project (Katello / Packaging). If its wrong there that project needs to be removed from redmine :D

mbacovsky commented 7 years ago

@Klaas- I don't think this change matches the process. The file is installer configuration for the specific scenario and can be tuned by the user. We don't wan't to lose the changes. If there are any changes to the default values or newly added extensions to the scenario in the following version, they should be implemented by a migration (https://github.com/Katello/katello-installer/tree/master/config/katello.migrations) The migrations are applied in %post phase of RPM installation automatically.

Klaas- commented 7 years ago

@mbacovsky foreman is handling those kind of files this way https://github.com/theforeman/foreman-packaging/blob/rpm/develop/foreman-installer/foreman-installer.spec#L69 are they incorrect or are those files not supposed to be handled the same way? irc consensus was that foreman behaviour is right :D

mbacovsky commented 7 years ago

@Klaas- the reason we decided to use migrations for changing the installer config is the complexity of upgrades. Users and the installer itself do changes to the installer config (interactivity, logging, installer arguments not related to the puppet modules and plugin modules paths) and we need to merge in our changes during the upgrade while not discarding the custom changes.

There were also plans to support cross scenario upgrades (Foreman ---> Foreman + Katello) which would be easier with migrations which seems to be more flexible solution.

It is not ideal that Foreman and Katello used different approach, but Kafo has support for both so technically it is okay. I'm not sure if it would be safe to change it for Katello now.

I'd be interested what others think and what is the experience with using migrations so far - @stbenjam, @bbuckingham, @ehelms?

sean797 commented 7 years ago

I spoke to @Klaas- about this on IRC earlier. As these files define the scenarios this is something that we ship and define, the user shouldn't be editing these files IMO. But I guess both approaches are valid.

@Klaas- what was the reason you realised this? Was there a problem with a scenario? Are we missing a migration?

mbacovsky commented 7 years ago

@sean797 AFAIK the installer stores with each run all the args given on the commandline. Args related to the puppet modules are stored in answer file, the rest is stored in the scenario file. This way installer can re-run previous installation with just foreman-installer with no args. Thus both the scenario config and the answer file are expected to be changed.

stbenjam commented 7 years ago

Don't worry about the redmine category nonsense, it's fine.

I think this approach is fine, but we should remove the migrations on those files from katello-installer as well. I might be missing something though, I'm not sure if there's any particular reason we took a different approach, but ours seems a bit more complicated than it needs to be.

mbacovsky commented 7 years ago

@stbenjam how do we keep the scenario settings during upgrade? Lets say the user changed e.g. the logging location. After the installer update the foreman-installer --upgrade will log to the default. Or do I miss something?

Klaas- commented 7 years ago

@sean797 in general it was no problem that prompted me to check this, I just noticed during upgrades that foreman replaces the files, katello doesn't and I started to wonder if it should be that way or not :) I think I replaced the files anyway when running "rpmconf -a" during my last upgrades

stbenjam commented 7 years ago

@mbacovsky If people are modifying the file then we should keep things the way they are I guess.

Klaas- commented 7 years ago

so if these files are meant to be modified by a user should the forman-installer spec file be updated to the same behaviour? having foreman and katello behave in a different way seems wrong to me

mbacovsky commented 7 years ago

@stbenjam, the users and the installer itself is expected to modify them so +1 form me to keep it as is @Klaas- I'm actually surprised that I didn't found any complains about loosing configuration during Foreman upgrades. But even for Foreman it seems the migrations are more safe way to update installer configs. It is likely that users do not customize the setup. Besides colors, log level and log location there is not much to customize

ehelms commented 7 years ago

Agreed - both the config and answers file should be modified through migrations and not replaced for existing users. Unless there are objections I will close this in the next few days.

Klaas- commented 7 years ago

@ehelms I'll close this, only question left is if we should try to streamline theforeman to not replace their yaml files aswell