Skip to content
This repository has been archived by the owner on Aug 29, 2018. It is now read-only.

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

Closed
wants to merge 1 commit into from

Conversation

Klaas-
Copy link
Member

@Klaas- Klaas- commented May 9, 2017

No description provided.

@theforeman-bot
Copy link

@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-
Copy link
Member Author

Klaas- commented May 9, 2017

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
Copy link
Contributor

@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-
Copy link
Member Author

Klaas- commented May 9, 2017

@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
Copy link
Contributor

@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
Copy link
Member

sean797 commented May 10, 2017

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

@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-
Copy link
Member Author

Klaas- commented May 11, 2017

@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
Copy link
Contributor

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

@Klaas-
Copy link
Member Author

Klaas- commented May 15, 2017

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
Copy link
Contributor

@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
Copy link
Member

ehelms commented Jun 8, 2017

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-
Copy link
Member Author

Klaas- commented Jun 13, 2017

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

@Klaas- Klaas- closed this Jun 13, 2017
@Klaas- Klaas- deleted the Klaas--patch-19481 branch April 25, 2018 08:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants