Fixes #31521 - structured plugin information in statuses API #8207

evgeni · 2020-12-16T14:16:34Z

No description provided.

theforeman-bot · 2020-12-16T14:16:43Z

Issues: #31521

app/services/ping.rb

ekohl

👍 for an explicit way. I do wonder about API compatibility here.

@ofedoren mind taking a look?

ehelms · 2020-12-16T14:38:21Z

app/services/ping.rb

+          name: plugin.name,
+          version: plugin.version,
+          author: plugin.author,
+          description: plugin.description,


Curious your thoughts on why send out author and description on a status API?

it was there before

foreman/app/registries/foreman/plugin.rb

Lines 233 to 235 in da7bca2

def to_s

"Foreman plugin: #{id}, #{version}, #{author}, #{description}"

end

and I didn't want to loose that data.

personally I only need id and version :)

Well, I doubt that someone would require those data... On hammer side we only show name and version anyway.

Speaking of name… the Foreman::Plugin object has both, name and id attributes, but both seem to be the same?

Hm.. what could be id of the plugin except its name anyway?.. AFAIK it's not possible to install two versions of a plugin and use both at the same time. I'd expect that id == name.

But apparently there can be such thing as id != name:

foreman/app/registries/foreman/plugin.rb

Lines 96 to 98 in 036bbe6

plugin = new(id)

if (gem = Gem.loaded_specs[id.to_s])

plugin.name gem.name

Yeah, name is the name of the gem, while id is what you put in when you register the plugin, so I'd actually say the id is the name and the name is worthless 😅

That said, let's make the data {name: plugin.id, version: plugin.version} and be done with it?

evgeni · 2020-12-16T14:38:25Z

we could also add it as a new entry, "plugin_data" or whatever, to keep it backwards compatible

evgeni · 2020-12-16T14:44:11Z

this will require changes to hammer, as it actually splits that thing itself:

https://github.com/theforeman/hammer-cli-foreman/blob/0b85edf6571b0d4f2f95123f9efa29e96c7b9fda/lib/hammer_cli_foreman/command_extensions/status.rb#L14-L19

theforeman/hammer-cli-foreman#553 takes care of this

ofedoren

@evgeni, surely this looks and is better to work with, thanks :)

@ekohl, about backward compatibility: currently information about plugins is being sent as an array of strings (which was quite dumb from my side to let it that way) as

{ "plugins" =>
  [ "plugin_a, version, etc",
    "plugin_b, version, etc"
  ]
}

To actually save compatibility we would need to add a new entry such as plugins_structured with this new data. This wouldn't break any automation (if there is any for such response), but will lead to data duplicity. Which is not so horrible for such amount, but.. is not so okay also.

I'd take a risk and change to @evgeni's version, but if others are okay with solution above I'm totally okay with having both entries.

ofedoren · 2020-12-17T14:49:35Z

app/services/ping.rb

+          name: plugin.name,
+          version: plugin.version,
+          author: plugin.author,
+          description: plugin.description,


Well, I doubt that someone would require those data... On hammer side we only show name and version anyway.

evgeni · 2020-12-17T15:27:50Z

I too would prefer not to duplicate the data, unless someone really wants that :)

ofedoren

LGTM, @evgeni :) Can't merge though :(

ofedoren · 2020-12-18T15:06:45Z

test/unit/plugin_test.rb

@@ -365,7 +365,12 @@ def test_register_status_extension
        api: {
          version: Apipie.configuration.default_version,
        },
-        plugins: [Foreman::Plugin.find(:foo)],
+        plugins: [


To be more accurate, I think it should be something like:

... plugin = Foreman::Plugin.find(:foo) ... plugins: [ { name: plugin.id, version: plugin.version } ] ...

I think it's a good practice to have exact values in your expected results. Otherwise you may miss a situation where a property may be nil for example. Essentially there is no assertion then.

I had the dynamic version before, and then had the same thought as Ewoud: lets test explicit values.

ekohl · 2020-12-18T15:12:08Z

@tbrisker can I ask you what you think about the API compatibility here?

ofedoren · 2020-12-18T17:02:52Z

@evgeni, now test fails are related :(

evgeni · 2020-12-18T17:05:03Z

Hah, that's because the version is taken from the Gem, but in the tests we mock the plugin and have no gem. Will have a thought how to fix that on Monday.

ares · 2020-12-19T10:36:22Z

The plugins info was introduced in Foreman 1.24. That's 3 versions already. I can imagine people use /ping endpoint to monitor Foreman. However I don't think people would monitor plugin versions specifically. I think it's a safe change for the better, but it is breaking backwards compatibility. If we decide to go with the duplication of the information, can we deprecate the existing way in release notes and drop it in Foreman 3.0? The problem is, we can't easily display the deprecation to the user, since we have no idea if they use this piece of the response. The other option is to be less dogmatic, ask on community or use RSS notification and drop it in e.g. one minor release if no one objects.

evgeni · 2020-12-19T11:30:47Z

[test foreman]

evgeni · 2020-12-19T13:37:32Z

`<class:ApplicationRecord>': uninitialized constant ApplicationRecord::ApipieDSL (NameError)

Looks unrelated?!

evgeni · 2020-12-19T19:20:18Z

Yeah, it is unrelated. See https://projects.theforeman.org/issues/31533

evgeni · 2020-12-20T16:59:44Z

[test foreman]
[test katello]

tbrisker

I'd be fine with modifying the existing API with an upgrade warning in this case. One could even argue that the previous response was a bug, calling to_s on the plugins instead of returning the hash of installed plugins. Perhaps we could also keep the authors and description so we can reuse it in the ui about page, but that's not crucial.

evgeni · 2020-12-21T14:19:20Z

I can add the details back in (my original version had them, but the feedback so far was "nobody needs that").

By upgrade warning you mean release notes in the docs? Can open a PR for that.

ekohl · 2020-12-21T14:23:08Z

Perhaps we could also keep the authors and description so we can reuse it in the ui about page, but that's not crucial.

I'm not sure if the about page should use ping. It reaches out to all backend services so if all you need to do is retrieve installed plugins, it's too heavy.

tbrisker · 2020-12-21T14:31:11Z

Perhaps we could also keep the authors and description so we can reuse it in the ui about page, but that's not crucial.

I'm not sure if the about page should use ping. It reaches out to all backend services so if all you need to do is retrieve installed plugins, it's too heavy.

good point, perhaps in the future we would want to use it so you can see that status in a nice ui, but we're getting ahead of ourselves. we can always add it later.

ofedoren · 2020-12-21T15:33:56Z

Perhaps we could also keep the authors and description so we can reuse it in the ui about page, but that's not crucial.

I'm not sure if the about page should use ping. It reaches out to all backend services so if all you need to do is retrieve installed plugins, it's too heavy.

AFAIU we use ping on the about page already.. Or you meant that the current way is not so good?

evgeni · 2021-01-04T06:31:40Z

upgrade warning in theforeman/theforeman.org#1752

tbrisker

Thanks @evgeni, @ofedoren - can you take a look at the hammer side please? I'm assuming we'll need to merge theforeman/hammer-cli-foreman#553 at the same time as this one?

ofedoren · 2021-01-04T15:55:17Z

@tbrisker, sure thing.

evgeni · 2021-01-04T16:13:50Z

Technically? Yes. Realistically, no, as hammer status (vs hammer ping) is IIRC not part of the pipelines ;)

ekohl · 2021-01-08T17:07:33Z

Technically? Yes. Realistically, no, as hammer status (vs hammer ping) is IIRC not part of the pipelines ;)

Was this intended for another PR?

evgeni · 2021-01-08T17:33:12Z

Technically? Yes. Realistically, no, as hammer status (vs hammer ping) is IIRC not part of the pipelines ;)

Was this intended for another PR?

No, it was an answer to tomers question whether it needs to go together with the hammer change

ekohl · 2021-01-11T15:37:12Z

Ok, but the Katello pipeline does call hammer ping (https://github.com/theforeman/forklift/blob/dbce1cfec5f658128518379679ed26f199724930/bats/fb-test-katello.bats#L9-L30), although technically nowadays that's generic for Foreman nowadays.

evgeni · 2021-01-11T15:55:33Z

Ok, but the Katello pipeline does call hammer ping (https://github.com/theforeman/forklift/blob/dbce1cfec5f658128518379679ed26f199724930/bats/fb-test-katello.bats#L9-L30), although technically nowadays that's generic for Foreman nowadays.

hammer ping != hammer status and the later is what's affected by the change here ;)

evgeni · 2021-01-12T14:29:14Z

hammer is merged now.

tbrisker · 2021-01-12T14:36:12Z

Thanks @evgeni et al!

Foreman 2.4 [1] started to return plugin information in a structured way, let's be nice and support that too :-) [1] theforeman/foreman#8207

theforeman-bot added Needs testing Not yet reviewed labels Dec 16, 2020

evgeni force-pushed the issue31521 branch from b702361 to 43994c6 Compare December 16, 2020 14:25

ekohl reviewed Dec 16, 2020

View reviewed changes

app/services/ping.rb Outdated Show resolved Hide resolved

evgeni force-pushed the issue31521 branch from 43994c6 to 1e1b4fd Compare December 16, 2020 14:32

ekohl previously approved these changes Dec 16, 2020

View reviewed changes

theforeman-bot removed the Not yet reviewed label Dec 16, 2020

ehelms reviewed Dec 16, 2020

View reviewed changes

evgeni dismissed ekohl’s stale review via c33dd60 December 16, 2020 18:27

evgeni force-pushed the issue31521 branch 2 times, most recently from c33dd60 to 786c9db Compare December 16, 2020 18:55

ofedoren previously approved these changes Dec 17, 2020

View reviewed changes

evgeni dismissed ofedoren’s stale review via 05695ac December 18, 2020 14:34

evgeni force-pushed the issue31521 branch 2 times, most recently from 05695ac to 02d468e Compare December 18, 2020 14:37

ofedoren previously approved these changes Dec 18, 2020

View reviewed changes

evgeni dismissed ofedoren’s stale review via 514236b December 18, 2020 19:06

evgeni force-pushed the issue31521 branch 2 times, most recently from 514236b to a46f631 Compare December 19, 2020 10:14

evgeni mentioned this pull request Dec 19, 2020

Refs #31533 - pin rake to < 13.0.2 to avoid test failures #8213

Merged

Fixes #31521 - structured plugin information in statuses API

313769b

evgeni force-pushed the issue31521 branch from a46f631 to 313769b Compare December 20, 2020 19:38

tbrisker approved these changes Dec 21, 2020

View reviewed changes

tbrisker approved these changes Jan 4, 2021

View reviewed changes

ares assigned tbrisker Jan 6, 2021

tbrisker merged commit 2bc006f into theforeman:develop Jan 12, 2021

evgeni mentioned this pull request Mar 10, 2021

support Foreman's structured plugin details theforeman/foreman-ansible-modules#1166

Merged

	def to_s
	"Foreman plugin: #{id}, #{version}, #{author}, #{description}"
	end

	plugin = new(id)
	if (gem = Gem.loaded_specs[id.to_s])
	plugin.name gem.name

Fixes #31521 - structured plugin information in statuses API #8207

Fixes #31521 - structured plugin information in statuses API #8207

Conversation

evgeni commented Dec 16, 2020

theforeman-bot commented Dec 16, 2020

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgeni commented Dec 16, 2020

evgeni commented Dec 16, 2020 • edited Loading

ofedoren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgeni commented Dec 17, 2020

ofedoren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl commented Dec 18, 2020

ofedoren commented Dec 18, 2020

evgeni commented Dec 18, 2020

ares commented Dec 19, 2020

evgeni commented Dec 19, 2020

evgeni commented Dec 19, 2020

evgeni commented Dec 19, 2020

evgeni commented Dec 20, 2020

tbrisker left a comment

Choose a reason for hiding this comment

evgeni commented Dec 21, 2020

ekohl commented Dec 21, 2020

tbrisker commented Dec 21, 2020

ofedoren commented Dec 21, 2020

evgeni commented Jan 4, 2021

tbrisker left a comment

Choose a reason for hiding this comment

ofedoren commented Jan 4, 2021

evgeni commented Jan 4, 2021

ekohl commented Jan 8, 2021

evgeni commented Jan 8, 2021

ekohl commented Jan 11, 2021

evgeni commented Jan 11, 2021

evgeni commented Jan 12, 2021

tbrisker commented Jan 12, 2021

evgeni commented Dec 16, 2020 •

edited

Loading