Fixes #11759 - fix include_javascript and lookup keys preparation

[iNecas]

include_javascript is not needed (and present) anymore, and lookup keys
factories changed behavior and we need to use some specialized factories.

[stbenjam]

The scope of the plugin makes me think these kinds of checks are going to get out of control really fast. Can we just not move to only supporting nightly now?

X should get bumped whenever incompatibility is introduced - which tends to be every release if you don't incorporate these checks.

For us, that means 1.0 would support Foreman 1.10, then 2.0 would support Foreman 1.11, and so on. That's how foreman_salt and foreman_bootdisk generally work, and I think it works well.

If we want to support more than one version at a time, the most important bug fixes can go to the previous version's branch. Nearly all other Foreman plugins do it this way, unless its trivial to have dual-version compatability, but I don't think that's the case for us.

[iNecas]

I agree with you in general, although I don't think cleaning the checks after we say that new version will be not 1.9 compatible is hard thing to do. We agreed on keeping the compatibility with the last stable release until there is something we need in core to support our feature, or there is some big change so that supporting both old and new versions would be inefficient. I don't think any of this changes fit into this category: this are really minor things. Being compatible with latest stable has also it's advantages: you can notice that we don't hit CI errors in the development process, that are caused by temporary breakages in nightlies: so we have time to work on compatibility with new version, while the development can continue without issues on 1.9 stable (I've seen many times this causing issues in Katello when running against nightlies).

We can talk about switching compatible versions at the end of iteration 2. So far, I would keep compatibility with 1.9.

[stbenjam]

Being compatible with latest stable has also it's advantages: you can notice that we don't hit CI errors in the development process

But we're also not running tests against develop so we're just getting this benefit at the expense of not having insight into changes in Foreman develop. We're also about to branch for 1.10, so it seems likely we'll GA REX against 1.10.

Testing against 1.9 and develop when I looked into it wasn't something Jenkins could easily be changed to do, as it makes some assumptions about plugins.

Why do we need to do things so differently than other plugins do?

I've seen many times this causing issues in Katello when running against nightlies.

Katello is a special case, but what's the problem with this?

The particular cases when Foreman changes break Katello nightly is important feedback, because we develop against develop - so we're ready for the next release. Generally we're aware of these before they're even committed to Foreman because of tests are done against Katello in Foreman PR's.

The cases where we're not aware well in advance is because of lack of test coverage in Katello on Foreman integration, etc, and isn't an argument to develop against stable.

[stbenjam]

If you really want Iteration 2 to be working on 1.9, we can merge this, but keep in mind 1.10 RC1 is less than a month away. We should have a plan for dropping 1.9 and moving to 1.10.

How about Iteration 3 we drop 1.9?

[ares]

I'd keep 1.9 compatibility until 1.10 hits stable. If we keep conditions in form of == 1.9 it's trivial to drop it later. Only really big effort to support both would persuade me it's better to drop 1.9 sooner. That being said it doesn't mean all merged features be available with 1.9.

[stbenjam]

I'd keep 1.9 compatibility until 1.10 hits stable.

I disagree, we should be ready to RC and GA concurrently with Foreman 1.10.

If we keep conditions in form of == 1.9 it's trivial to drop it later.

I didn't say it wasn't trivial, I'm just suggesting it sets a precedent. TODO's and FIXME's are rarely done "later".

[ares]

I disagree, we should be ready to RC and GA concurrently with Foreman 1.10.

why do you think we won't be? we found this issue with nightly quite soon, since we develop against nightly... once there's RC we could just run tests manually once, I don't expect too many incompatible changes after that point

but if you think it's worth of cutting 1.9 compatibility, I don't insist

[stbenjam]

It doesn't make sense to me that we test our PR's against 1.9, and get accidental 1.10 coverage just by developers running it on their machines.

When 1.10 stable is out, we're going to be in the same situation, trying to write code that supports develop and 1.10 at the same time.

I'd prefer to only develop new features against develop, and maintain a stable branch for backporting bug fixes, and not doing this version checking. It's going to get worse when we support Reports STI, Host Status, etc. It's simple now, but its not going to be.

[stbenjam]

But if everyone else disagrees, then I withdraw my objection, and ACK to merge this. The PR itself works fine.

[iNecas]

I would be ok with iteration 3 dropping 1.9 (lets talk about it on planning for iteration 3), part of dropping 1.9 is cleaning up the conditions (the difference between this and regular TODO/FIXME is not event that would imply the check, while dropping a supported release is a clear impuls for removing the conditional code)

[iNecas]

Yes, when starting to deal with reports STI or global status, it would be a different case (and gets into the category of "there is some big change so that supporting both old and new versions would be inefficient", it's no the case right now, and all the work for iteration 2 should work on both versions. Thanks for the discussion. Merging