moving configuration files so that local conf is ignored by git, updated package, bumped version to 0.0.11. Fixes T144
ClosedPublic

Authored by tflink on Apr 30 2014, 5:10 PM.

Details

Summary

T144 is a rather large annoyance. I fixed it so that moving between branches doesn't require local config reset

Test Plan

local package has been built, works

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
kparal requested changes to this revision.May 2 2014, 9:44 AM
kparal added subscribers: pschindl, jskladan.

When I talked about yumrepoinfo.conf in T144, I haven't realized the consequences. If we rename it, it is no longer found by yumrepoinfo.py lookup. Which means libtaskotron doesn't know any existing yum repos. And therefore tasks like upgradepath no longer run. This can be solved by copying yumrepoinfo.conf.example -> yumrepoinfo.conf or yumrepoinfo.conf.example -> /etc/taskotron/yumrepoinfo.conf. But by default, after a clean checkout, it doesn't work. And I believe we don't want that.

The solutions I see:

  1. Create .example just for taskotron.yaml. This is easy to do, but it introduces inconsistency in our project and we're soon going to hit the same issue this patch is meant to solve, once someone needs the adjustments made in yumrepoinfo.conf instead of taskotron.yaml. I.e. shortsighted.
  2. Have an internal (third) copy of yumrepoinfo.conf to be used as a fallback when no other config files are available. Simple enough to do, but our project will be flooded with config files copies :-)
  3. Don't read config files from local checkout at all. We will clearly explain in config files' headers that all information is read only from /etc. That will stop people from modifying local checkout files and we don't need to append .example suffixes. People will need to adjust the files in /etc/. I'm missing an disadvantage here - Tim, could you explain why you need to modify local checkout config files instead of using /etc/ ones? Thanks.
  4. (possible replacement for 2) ) Read the .example files as well, as a backup when no other file exists. This saves us from creating internal copies inside libtaskotron/. However, the whole config business is a made even more unclear/messier - for example, I wouldn't expect that removing an .example file could change software behavior. In this case, it could.

Does someone see a more elegant solution? (CC to @mkrizek, @jskladan, @pschindl)
At present, the best ways forward in my view seem to be 3) or 2).

tflink added a comment.May 2 2014, 1:55 PM
  1. Create .example just for taskotron.yaml. This is easy to do, but it introduces inconsistency in our project and we're soon going to hit the same issue this patch is meant to solve, once someone needs the adjustments made in yumrepoinfo.conf instead of taskotron.yaml. I.e. shortsighted.

Yeah, it would make using non-standard or local yum repos problematic.

  1. Have an internal (third) copy of yumrepoinfo.conf to be used as a fallback when no other config files are available. Simple enough to do, but our project will be flooded with config files copies :-)

This seems like an invitation to miss a config update and I'm not sure we'd really gain much for the extra complexity.

  1. Don't read config files from local checkout at all. We will clearly explain in config files' headers that all information is read only from /etc. That will stop people from modifying local checkout files and we don't need to append .example suffixes. People will need to adjust the files in /etc/. I'm missing an disadvantage here - Tim, could you explain why you need to modify local checkout config files instead of using /etc/ ones? Thanks.

I fail to see the difference between this and keeping things as they are with this patch - people still won't be able to run tasks that require yumrepoinfo.conf or libtaskotron.yaml on a fresh checkout. The difference is that instead of copying files inside the local checkout, they have to create config files in /etc/.

I prefer the local configuration option because

  • It allows developers to change configuration while staying within the git checkout. With a configured virtualenv, the checkout effectively contains everything needed for development and I don't have to go hunting for config files
  • Requiring config files in etc to use a git checkout seems weird, at best.

Also, with the code as is, local config takes precedence over /etc/ config and changes to those /etc/ files wouldn't be a solution.

  1. (possible replacement for 2) ) Read the .example files as well, as a backup when no other file exists. This saves us from creating internal copies inside libtaskotron/. However, the whole config business is a made even more unclear/messier - for example, I wouldn't expect that removing an .example file could change software behavior. In this case, it could.

Yeah, I don't like the idea of reading *.example files either. That seems like a rather bad idea to me.

Of these options, the patch as is and 3. don't work immediately out of a fresh git checkout for all tasks which, I think, is the concern. My question is whether that's really such a big deal - it's a bit of a bigger concern until libtaskotron stabilizes and we have better available packages for people to use but I'm not sure that we really want to cater to the use case of "using a fresh git checkout on new software without at least reading the README"

I'd rather keep the concept of local config and .example files in this patch - would adding a WARNING/ERROR if config parsing finds no available config files help? Something like

WARNING: no configuration files for <configfile> found in ./conf/ or /etc/taskotron/. All available features and tasks may not work without valid configuration

substituting yumrepoinfo.conf or libtaskotron.yaml for <configfile> as appropriate. That way, we maintain the ability to use local config and there's less chance of someone using an unconfigured install and wondering why it doesn't work.

tflink added a comment.May 2 2014, 2:05 PM

It occurs to me that there is another problem with this patch. Specifically, the use of %config(noreplace) on yumrepoinfo.conf. We're going to have to update the package when repos change unless we expect users to do their own config file updates and I suspect that creating /etc/taskotron/yumrepoinfo.conf.rpmnew is going to be frustrating for folks.

One possible solution would be to create default config files in /usr/share/taskotron/conf which are overridden by files in /etc/taskotron which does add some complexity but for the installed case, adding a directory to the existing search directories shouldn't be too bad as long as the /usr/share/taskotron/conf files are the lowest priority. That way, we can update yum repo configuration without worrying about stomping on existing config in /etc/ for existing installs.

kparal added a comment.May 5 2014, 2:49 PM

This seems like an invitation to miss a config update and I'm not sure we'd really gain much for the extra complexity.

I assumed the yumrepoinfo.conf.example would be symlinked to the internal one, so that only a single copy would exist.

Also, with the code as is, local config takes precedence over /etc/ config and changes to those /etc/ files wouldn't be a solution.

No, it's the other way around. If there's a file in /etc, the local file is ignored.

Isn't this a far worse problem for your workflow? Once someone installs libtaskotron and also creates a local git checkout, the local checkout configuration will never be used. Originally, I wanted to give priority to /etc, because local conf file always existed. If we move to .example files, it makes sense to revert it - give priority to local file, if it exists, and only then check /etc.

We're going to have to update the package when repos change unless we expect users to do their own config file updates and I suspect that creating /etc/taskotron/yumrepoinfo.conf.rpmnew is going to be frustrating for folks.

Why don't we simply use %config instead of %config(noreplace)? That way users will always have the most recent repo definitions and if they want, they can adjust it/merge it with .rpmsave.

I don't see any difference between %config(noreplace) and using /usr/share/taskotron/conf. In both cases:
a) if they haven't modified the config file, it will be updated [1]
b) if they have modified the config file (or overridden it in the latter case), they will need to merge the changes
[1] http://www-uxsup.csx.cam.ac.uk/~jw35/docs/rpm_config.html

My question is whether that's really such a big deal - it's a bit of a bigger concern until libtaskotron stabilizes and we have better available packages for people to use but I'm not sure that we really want to cater to the use case of "using a fresh git checkout on new software without at least reading the README"

I really see this as a big hurdle for anyone playing with this. I would rather to devote some time and keep internal yumrepoinfo.conf in sync with the example version, than to require everyone who checks out our project to perform some manual steps before they can use it. My drive is to make the check development as simple and approachable as possible. If we ever ended up with an error message like the one you proposed, I believe it should halt the whole execution and exit. Otherwise people won't notice that among other messages. But I would prefer something working out of the box... <kparal thinking>

tflink added a comment.May 5 2014, 4:09 PM

I assumed the yumrepoinfo.conf.example would be symlinked to the internal one, so that only a single copy would exist.

What's the difference between that and reading the *.example files directly?

Why don't we simply use %config instead of %config(noreplace)?

I'm fine with either solution - the effect I'm interested in is updating config files for the default case without completely stomping on any custom changes.

I really see this as a big hurdle for anyone playing with this. I would rather to devote some time and keep internal yumrepoinfo.conf in sync with the example version, than to require everyone who checks out our project to perform some manual steps before they can use it. My drive is to make the check development as simple and approachable as possible. If we ever ended up with an error message like the one you proposed, I believe it should halt the whole execution and exit.

Yeah, I think it would have to for the cases where yum repo info is required. If the repo data doesn't exist in currently available data, the task in question should fail with an error.

Otherwise people won't notice that among other messages. But I would prefer something working out of the box... <kparal thinking>

I agree with you that in most cases, it should work out of the box. However, are most task authors going to be running libtaskotron from git? I would prefer the yum install libtaskotron case, even though we don't have libtaskotron in the Fedora repos yet. In that case, the *.example issue would be handled by the defaults in /usr/share or however we decide to handle config file installation in the spec file and users shouldn't have to do any manual steps to make everything work with default values.

That being said, I'm starting to wonder if I just need to change my default workflow to use /etc/ files instead of modifying stuff in ./config. As long as the /etc/ files aren't required for operation and it's possible to modify configuration via the local in-tree files (not recommended, but do-able), my primary concerns about configuration files for dev environments are addressed.

Any objection to backing out the *.example changes and just updating the specfile to use %config(noreplace) on taskotron.yaml and %config on yumrepoinfo.conf?

kparal added a comment.May 6 2014, 2:12 PM

Any objection to backing out the *.example changes and just updating the specfile to use %config(noreplace) on taskotron.yaml and %config on yumrepoinfo.conf?

None at all.

But I'd like to support your use case of local config files, @mkrizek is a heavy supporter of it :) We discussed it locally, and we have a suggestion:

  1. Create a separate RPM package libtaskotron-yumrepoinfo, which will contain /etc/taskotron/yumrepoinfo.conf
  2. Make libtaskotron require libtaskotron-yumrepoinfo.
  3. In yumrepoinfo.py, first search for ../conf/yumrepoinfo.conf, then search for /etc/taskotron/yumrepoinfo.py. Use the first config file found.
  4. In git checkout, add conf/README and describe that local config files can be used - not just taskotron.yaml, but also yumrepoinfo.conf copied from /etc.

The benefits include:

  • you can override all config files with a version placed in git checkout
  • we can rely that some yumrepoinfo will always be available, because the dependency will install it (and we'll raise an error if the file isn't there)
  • we can update yumrepoinfo without releasing a new version of libtaskotron (we estimated it happened circa 6 times a year before just because of repoinfo changes - two new releases, two EOLs, two branches from rawhide).

And the downside:

  • we need to manage a separate RPM package

What do you think?

tflink added a comment.May 6 2014, 2:28 PM

I'm not sure what problem your yumrepoinfo package suggestion solves. If I'm understanding correctly, we'd still have ./conf/taskotron.yaml and ./conf/yumrepoinfo.conf in git. While the local config method technically works, we're back to the same problem of merge and branch switch issues with local config changes and the eventual "whoops, that config change was supposed to stay local"

Or are you and @mkrizek talking about keeping the *.example concept in git? If so, don't we still have the same problem of a fresh git clone not working out of the box without config manipulation?

kparal added a comment.May 6 2014, 3:51 PM

Or are you and @mkrizek talking about keeping the *.example concept in git?

Yes, exactly. Sorry, haven't really explained that. We were thinking about the possibility to have ./conf/<config> (non-existent by default), and having .example files instead. Like this:

libtaskotron
|- conf
   |- taskotron.yaml.example
   |- instructions.txt

instructions.txt:

You can override the global config files from /etc/taskotron here, this directory has a preference.
Copy taskotron.yaml (from the .example file) or yumrepoinfo.conf (from /etc/taskotron) here and 
make your modifications. Those files will be ignored by git.

If so, don't we still have the same problem of a fresh git clone not working out of the box without config manipulation?

In README (and in libtaskotron.spec), along with other dependencies, we would specify taskotron-yumrepoinfo. We would provide this package in our QA repository, and later it would be available in Fedora proper. Since this is just a simple config file, there's no need to use "git checkout version" for this. Everyone would simply install the package.

tflink updated this revision.May 7 2014, 5:20 AM
  • splitting configuration files out into a subpackage, updating documentation and changing precedence of confguration file lookup
kparal accepted this revision.May 7 2014, 11:59 AM

Yes, I think this should work well. There are a few details to fix, see comments below, but otherwise this can be shipped. Once this is committed, we need to build the packages and then send an email to qa-devel asking everyone to install libtaskotron-config. I'll prepare a patch to abort execution if yumrepoinfo.conf is not found anywhere.

readme.rst
38

This is already in requirements.txt. I guess it should be either here or in that file.

168

.exampe typo

tflink added inline comments.May 7 2014, 1:00 PM
readme.rst
38

The only reason I added it is that resultsdb_api isn't in pypi and figured that it'd be easier to install from a yum repo than to clone a repo and installing to a virtualenv.

Is there a problem with resultsdb_api being listed in both places other than the repetition?

kparal added inline comments.May 7 2014, 2:04 PM
readme.rst
38

https://pypi.python.org/pypi/resultsdb_api

Is there a problem with resultsdb_api being listed in both places other than the repetition?

No, I don't think so. It just might be bit confusing whether we require _both_ versions to be installed (from PyPI and from RPM).

tflink added inline comments.May 7 2014, 2:33 PM
readme.rst
38

/me eats words. Will remove this line from the code I push to develop

tflink closed this revision.May 9 2014, 1:57 PM

Closed by commit rLTRN25632ba5e506 (authored by @tflink).