search

openSUSE / doc-ci

Continuous integration for SUSE documentation
4 stars 6 forks source link

Avoid hardcoding Geekodoc in travis.sh #18

Closed tomschr closed 4 years ago

tomschr commented 5 years ago

Situation

Currently, the travis.sh script uses a hard-coded path for Geekodoc. This is not always wanted as there may be situations to validate against DocBook (for example, Cloud) and not Geekodoc.

Inspired by Alex.

Solution

If the environment variable DOCBOOK5_RNG_URI is exported, the script can use the exported value to write the DAPS configuration file. If there is no DOCBOOK5_RNG_URI variable exported, use Geekodoc.

Update: I've realized too late that this can be done by adding the DOCBOOK5_RNG_URI variable to a DC file as well. Maybe it will be nevertheless useful if you can't change the DC file.

ghost commented 5 years ago
  1. The fix from #20 is broken. It now uses DocBook 5 by default. Not Geekodoc. Edith I misread the commit, it is buggy but not outright broken.
  2. This is way way more complicated than it needs to be. A simple switch for DocBook 5-current/GeekoDoc would have been enough here. Having to guess at the availability of RNG paths in some container image is pretty nonsensical. And we do not ship much else besides DocBook 5 and GeekoDoc anyway.
tomschr commented 5 years ago

The idea was born with the idea in mind to support validation with DocBook and Geekodoc without touching any file in doc repos. I realized later that it would have be done better with adding only the DOCBOOK5_RNG_URI environment variable into the DC file.

However, the current implementation does not change the default. It's still Geekodoc, it doesn't change the default behavior.

Having to guess at the availability of RNG paths ...

I'm not sure what are you complaining about. No writers needs to know that except us tech people It's a "constant" as long as we don't change the paths in the respective packages.

If you disagree... feel free to provide a better implementation.

ghost commented 5 years ago

However, the current implementation does not change the default. It's still Geekodoc, it doesn't change the default behavior.

I initially misread the patch. However, while the default is indeed still Geekodoc, the fallback is now DocBook 5.1 for some reason.

I'm not sure what are you complaining about.

It's 30+ lines for something that should have better been 5, to be simpler to test/use for everyone. It ignores the existing way of setting environment variables in favor of creating a new one. And it is buggy (see above). Also, some of the comments in the patch do not match reality.

tomschr commented 5 years ago

It's easy: if you think it's bad, just revert the change.

ghost commented 4 years ago

Ok, done.