Fixes #12568 - Add module versions to /version

[shlomizadok]

Adds header Proxy_version to RootApi calls (/features, /version) so it could be later used to display in Foreman see #12605

[ohadlevy]

i would test another path :) but 👍

[shlomizadok]

It would work only with /version and /features. I guess I could add another test for features too

[ohadlevy]

what about plugin versions?

[domcleal]

Please use a new Redmine ticket under the smart proxy project, not a Foreman one (http://projects.theforeman.org/projects/foreman/wiki/Reviewing_patches-commit_message_format#Notes).

[dmitri-d]

Why adding a header -- we already have /version api end-point? If we are going to add such a header, it should be applied universally, not just one module.

[ohadlevy]

I agree, and if we are going to touch the version, then imho we should also include plugin versions. (or per feature)

[dmitri-d]

/features can be extended to include plugin versions; Otherwise we are duplicating information in /version and /features calls. I assume that the driver behind this change is to retrieve all information needed to show the screen in the ticket in one call. If this is the case, I wouldn't bother with the header, extend /features to include root module and add version information to it.

[ohadlevy]

i think while you might be right, we might need to do conditional info per API call, therefore it does make sense to have the version handy. (instead of saving it on foreman db or similar). we already do the same in foreman.

[dmitri-d]

@ohadlevy: not sure I'm following. You mean you want to track incompatible api changes via plugin versions? It's one way to do it, but this is going to turn messy in no time. I would suggest implementing proper api versioning; I think it will have to be per-module though.

[lzap]

I like the header because I was actually about to submit similar functionality for discovery image (http://projects.theforeman.org/issues/12412). The problem FDI have is I need to find out which endpoint is FDI configured with - if its a proxy or foreman instance.

My idea is to keep the header and also add one extra one: "Foreman_Endpoint" with hardcoded "smart-proxy" in it? I could do similar patch in Foreman Core returning "foreman" so single GET / would easily give us information if this is Foreman or Proxy.

Another option would be to create a "status" or "version" call with same paths on both, but the header approach looks like a better one to me. Opinions?

[lzap]

Nitpick: http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html looks like headers follow this naming conventions:

X-Proxy-Version
X-Foreman-Endpoint

[dmitri-d]

Again, what are we trying to accomplish? If we need module version information (to show it to users, for example), I suggest extending /features -- data that are used together should have similar representation, as opposed to mixing up meta-data (headers) and json-serialized data.

If we need versioned API, then I would be in favour of using headers, but these should be separate from module versions:

• Each module will need to maintain at least a couple of API versions (latest, and one or more older versions for compatibility). We should probably have a notion of 'default' version.
• incompatible api changes are less frequent than plugin releases).

In short, I don't want to piggyback on module version information to track API changes; data representations such as http headers and json are employed for different purposes, let's use them accordingly.

[shlomizadok]

I agree with @witlessbird.
However, if we extend /features is there a chance of breaking things on the Foreman side?
A suggestion to extended /features may look like:

[
    "dhcp",
    "dns",
    "openscap",
    "tftp",
    {
        "versions": [
            {
                "openscap": "0.5.0"
            },
            {
                "foreman_proxy": "1.11.0"
            },
            {
                "dns": "1.11.0"
            },
            {
                "tftp": "1.11.0"
            },
            {
                "dhcp": "1.11.0"
            }
        ]
    }
]

Does that make sense?

[dmitri-d]

@shlomizadok: good point; This is still incompatible though. I think /version endpoint is much easier to extend in backwards-compatible way:

  {:version => '1.11',
   :modules => {
                         :tftp => '1.11',
                         :openscap => '0.5.0',
                         ... }
  }

[shlomizadok]

@witlessbird - I have added the module versions to /version.
I have left the proxy-version header as the discussion was inconclusive. Shall I drop it?

[dmitri-d]

@shlomizadok: nods, pls. drop the header.

[shlomizadok]

Done. 👍 - thanks for the review @witlessbird

[dmitri-d]

you know exactly what you are getting back (you stubbed enabled_plugins method). You can hardcode the expected reply, which, I think, will read better.

[shlomizadok]

Right, but hard-coding will enforce me to change if the stubbing ever changes.
I think it is ok to leave this as is

[ohadlevy]

Q: does this means we are breaking the API ? what about foreman code where the proxies are not updated, should we start talking about API versions?

[dmitri-d]

Adding another key to this hash shouldn't break existing clients: they expect hash, but are only aware of :version key, which remains. Newer clients can use both keys.

We should probably add versioning to api; if anything, this would give plugin authors more freedom/flexibility to make breaking changes in their api.

[dmitri-d]

Merged in 1128d93. Thanks, @shlomizadok!