Add Vmdb::Plugins#versions #17755

Fryguy · 2018-07-24T17:36:00Z

ManageIQ/manageiq-v2v#422
https://bugzilla.redhat.com/show_bug.cgi?id=1594348

With these override combinations:

# 1. git based, pointing to the master branch (default Gemfile reference)
gem "manageiq-content", :git => "https://github.com/ManageIQ/manageiq-content", :branch => "master"

# 2. git based, pointing to my_branch_1 branch
override_gem "manageiq-providers-ovirt", :git => "https://github.com/Fryguy/manageiq-providers-ovirt", :branch => "my_branch_1"

# 3. git based, pointing to my_tag_1 tag
override_gem "manageiq-providers-amazon", :git => "https://github.com/Fryguy/manageiq-providers-amazon", :tag => "my_tag_1"

# 4. git based, pointing to sha 5598b27
override_gem "manageiq-providers-openstack", :git => "https://github.com/ManageIQ/manageiq-providers-openstack", :ref => "5598b27"

# 5. path based, with git, on the master branch
override_gem "manageiq-ui-classic", :path => File.expand_path("~/dev/manageiq-ui-classic")

# 6. path based, with git, on my_branch_1 local branch
override_gem "manageiq-api", :path => File.expand_path("~/dev/manageiq-api")

# 7. path based, with git, on my_tag_1 tag
override_gem "manageiq-providers-azure", :path => File.expand_path("~/dev/manageiq-providers-azure")

# 8. path based, with git, on sha b7a0e17
override_gem "manageiq-v2v", :path => File.expand_path("~/dev/miq_v2v_ui_plugin")

# 9. path based, without git
override_gem "manageiq-providers-vmware", :path => File.expand_path("~/dev/manageiq-providers-vmware")

Example output:

[1] pry(main)> Vmdb::Plugins.versions
=> {ManageIQ::Api::Engine                     => "my_branch_1@9d143228",  # #6
    ManageIQ::AutomationEngine::Engine        => "master@06af67f5",
    ManageIQ::Content::Engine                 => "master@54eb45d3",       # #1
    ManageIQ::GraphQL::Engine                 => "master@5f68621f",
    ManageIQ::Providers::Amazon::Engine       => "my_tag_1@414002be",     # #3
    ManageIQ::Providers::AnsibleTower::Engine => "master@205e4b5d",
    ManageIQ::Providers::Azure::Engine        => "my_tag_1@9813132",      # #7
    ManageIQ::Providers::Foreman::Engine      => "master@1736186b",
    ManageIQ::Providers::Google::Engine       => "master@3b86b8c2",
    ManageIQ::Providers::Kubernetes::Engine   => "master@0968969d",
    ManageIQ::Providers::Kubevirt::Engine     => "master@c31b0650",
    ManageIQ::Providers::Lenovo::Engine       => "master@1dd5282f",
    ManageIQ::Providers::Nuage::Engine        => "master@dbd2a1bb",
    ManageIQ::Providers::Openshift::Engine    => "master@16a47af1",
    ManageIQ::Providers::Openstack::Engine    => "5598b270",              # #4
    ManageIQ::Providers::Ovirt::Engine        => "my_branch_1@824cb700",  # #2
    ManageIQ::Providers::Redfish::Engine      => "master@4effccdc",
    ManageIQ::Providers::Scvmm::Engine        => "master@b0e66d16",
    ManageIQ::Providers::Vmware::Engine       => nil,                     # #9
    ManageIQ::Showback::Engine                => "master@a558f35b",
    ManageIQ::UI::Classic::Engine             => "master@9a6582d17",      # #5
    ManageIQ::V2V::Engine                     => "b7a0e17"}               # #8

@bdunne Please review
cc @AparnaKarve @priley86

Fryguy · 2018-07-24T17:37:31Z

While the method is pretty simple, I'm wondering if the output should be more detailed instead of just the version string? That is, we could have the value be a hash with component pieces. Additionally, there are a number of other methods in this class that enumerate all of the plugins and their signatures are all different, so I'm wondering if we should unify that.

chessbyte · 2018-07-24T17:47:31Z

@Fryguy Would it make sense to create specs/tests for this new functionality based on your example?

Fryguy · 2018-07-24T17:50:16Z

I was thinking that, but I have no idea how. This depends on bundler's view of gems. So, I'd have to write specs that muck with bundler's internals. On top of that, the gems are constantly moving, so I couldn't hardcode any SHAs into the test. Any thoughts on how to mock this maybe?

EDIT: Forgot to mention that the path based ones shell out to git, so I'd need to create fake repos somewhere attached to gem specification paths, which seems scary

bdunne · 2018-07-24T18:02:35Z

Should we show the version from the gemspec too?

For #9, should we list the path or something rather than nil?

Fryguy · 2018-07-24T18:07:20Z

Should we show the version from the gemspec too?

I added a line of code for that if it's a normal gem, but we don't have any plugins released as gems atm.

For 9, should we list the path or something rather than nil?

I thought about that, but the version is still just nil. This was actually why I asked this ☝️ #17755 (comment) . I could see this returning {:path => "/path/to/gem", :version => nil}, but if I make them all return path, then it starts to get weird, because it will point to the gem path location, which we probably don't want to show.

Fryguy · 2018-07-24T19:03:46Z

@chessbyte Ok, I think I found something workable. I can create fake repos in tmpdir, and stub out the specs that way. It's rather invasive, but I think it will work.

Fryguy · 2018-08-03T21:47:41Z

@chessbyte @bdunne Updated with specs.

The way the specs work is that it creates a temp dir that represents the plugin, and if that directory is a git directory it initializes it with 2 commits, then based on some option, checks out the first commit as a branch, a tag, or a detached HEAD. Additionally, the test stubs out the Bundler Gem specification with this temp directory. Then, it verifies that the .version private method works as expected for every combination in the matrix.

NickLaMuro

My unsolicited review.

Looks cool though, so just some things that I noticed while peeking at the code.

NickLaMuro · 2018-08-03T23:01:31Z

lib/vmdb/plugins.rb

+        if engine.root.join(".git").exist?
+          branch = sha = nil
+          Dir.chdir(engine.root) do
+            branch   = `git rev-parse --abbrev-ref HEAD 2>/dev/null`.strip.presence


Not that this (or the next three lines) is wrong, but is is possible to call out to bundler for some of this so we aren't doing what seems like the same work that bundler already does again?

It would probably require re-wrapping the bundler spec in another one, but might save duplicating some work with the shelled out methods. Just a thought.

This section is for the "path, with git". In other words, this is for developers who use a :path reference in the Gemfile, and that path happens to have a .git directory in it. In this case (as opposed to the :git in a Gemfile case) Bundler does not have any git information and we need to fetch it ourselves.

@Fryguy I am aware, but I assume determining the branch and sha is nothing different than what Bundler::Source::Git is doing, and the only reason Bundler isn't doing it is because the :path flag is telling it that it doesn't need to.

Again, this would require re-wrapping the specification, and might not be worth it if it is more code to do and calling into a private API, which might break. But was curious if you had tried, and you could possibly re-use the code from above.

When I have a Bundler::Source::Path it doesn't have this data in it. I guess I could manufacture a Bundler::Source::Git though, but I don't know what that entails.

I tried replacing my code with the Bundler::Source::Git::GitProxy, but it didn't save much code, as I still had to handle removing HEAD and the GitProxy doesn't do the tag lookup. Bundler::Source::Git works the way it does because the user is specifying most of that information in the Gemfile, so it doesn't have to look it up. It really only needs the GitProxy to find the SHA, which is does the same way I do it https://github.com/bundler/bundler/blob/master/lib/bundler/source/git/git_proxy.rb#L193 (they have --verify, but I don't need it because I'm passing HEAD which will exist)

NickLaMuro · 2018-08-03T23:02:37Z

lib/vmdb/plugins.rb

+          branch = sha = nil
+          Dir.chdir(engine.root) do
+            branch   = `git rev-parse --abbrev-ref HEAD 2>/dev/null`.strip.presence
+            branch   = nil if branch == "HEAD"


Can you do this instead:

if branch != "HEAD" branch = `git rev-parse --abbrev-ref HEAD 2>/dev/null`.strip.presence end

Instead of the above to lines. Then you aren't running the line above to just throw it away in the next line.

I don't yet have branch to do the conditional that way

I am throwing it away on purpose because I don't want HEAD as the branch name. These 3 lines fetch the "name" of the branch, however if the branch is "HEAD", then we are actually in a detached HEAD state, so we need to check if we are on a tag instead. I set it to nil because we don't want HEAD to be our branch name, and if the tag is nil it will just stay nil.

Oh right... clearly I was drunk when I made that comment. Carry on.

NickLaMuro · 2018-08-03T23:05:27Z

lib/vmdb/plugins.rb

+            branch   = nil if branch == "HEAD"
+            branch ||= `git describe --tags --exact-match HEAD 2>/dev/null`.strip.presence
+
+            sha = `git rev-parse HEAD 2>/dev/null`.strip.presence[0, 8]


In this line and the line above, is presence needed?

I see why it is there for the first line so the ||= can work, but I don't get why it is used after that. Also, in this last line, it seems like it might cause an issue if it returns nil and tries calling [0, 8] on it.

Oh right...This line is backwards. I am using presence to enforce nils for the compact.join later.

NickLaMuro · 2018-08-03T23:13:21Z

spec/lib/vmdb/plugins_spec.rb

+      allow(Bundler::Source::Git).to receive(:===).with(source).and_return(type == :git)
+      allow(Bundler::Source::Path).to receive(:===).with(source).and_return(type != :git)
+
+      method = (type == :path ? :with_temp_dir : :with_temp_git_dir)


Pedantic: Do we normally surround ternaries that end up in an assignment with parens?

(I don't, which is why I am even asking)

I usually only do when I introduce a comparison operator that has an = in it.

method = type == :path ? ... reads funny to me.

NickLaMuro · 2018-08-03T23:17:28Z

spec/lib/vmdb/plugins_spec.rb

+        end
+
+      allow(Bundler::Source::Git).to receive(:===).with(source).and_return(type == :git)
+      allow(Bundler::Source::Path).to receive(:===).with(source).and_return(type != :git)


So been thinking about this. To avoid some of the stubbing, could we possibly use bundler init (or more preferably, a call the Bundler API to do the equivalent) to avoid this stubbing?

Thinking about it, might not work with the current Bundler.env which is why you are stubbing in the first place, just trying to avoid that subbing if possible.

I hadn't considered it, but I'm concerned about infecting the current test process.

carbonin · 2018-08-06T13:36:14Z

@Fryguy We will need to make git an explicit dependency in manageiq-appliance-build and manageiq-pods for this.

EDIT: Looks like it's already there in pods, but I don't think we're installing it downstream for some reason cc @simaishi

simaishi · 2018-08-06T13:47:33Z

I think the only reason we're currently installing git on upstream pods is to use source from git, which downstream images don't need.

@Fryguy What's the plan for downstream for this feature? We currently use BUILD file and don't run git rev-parse.

Fryguy · 2018-08-06T14:10:06Z

@simaishi @carbonin This code works when git is not installed, so I am not presuming it will be installed. In this case, it will be similar to this test https://github.com/ManageIQ/manageiq/pull/17755/files#diff-f415e2c63db9a92977dd7f3cb5d91a59R198

This code is similar in spirit to https://github.com/Fryguy/manageiq/blob/7ac24eb92dee3f8ebdf56e4741c822feb9a9caad/lib/vmdb/appliance.rb#L118-L130 where we do use git when the BUILD file is not present.

EDIT: I verified this works on downstream appliances...it goes down the Bundler::Source::Git leg of the code.

ManageIQ/manageiq-v2v#422 https://bugzilla.redhat.com/show_bug.cgi?id=1594348

miq-bot · 2018-08-06T17:51:26Z

Checked commit Fryguy@dbd79bf with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

bdunne mentioned this pull request Jul 30, 2018

[RFE] Add the /api/product_info route with product and branding info ManageIQ/manageiq-api#438

Merged

Fryguy force-pushed the plugin_versions branch from 47e581b to d1a6f97 Compare August 3, 2018 21:41

Fryguy force-pushed the plugin_versions branch 6 times, most recently from 368d2e1 to 7ac24eb Compare August 3, 2018 22:13

NickLaMuro reviewed Aug 3, 2018

View reviewed changes

Fryguy added the enhancement label Aug 6, 2018

Fryguy assigned bdunne Aug 6, 2018

Add Vmdb::Plugins#versions

dbd79bf

ManageIQ/manageiq-v2v#422 https://bugzilla.redhat.com/show_bug.cgi?id=1594348

Fryguy force-pushed the plugin_versions branch from 7ac24eb to dbd79bf Compare August 6, 2018 17:48

bdunne merged commit 97b4735 into ManageIQ:master Aug 6, 2018

bdunne added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 6, 2018

bdunne added the gaprindashvili/no label Aug 6, 2018

Fryguy deleted the plugin_versions branch August 7, 2018 13:36

AparnaKarve mentioned this pull request Aug 7, 2018

Display Plugin commit hashes in the About Modal ManageIQ/manageiq-ui-classic#4423

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Vmdb::Plugins#versions #17755

Add Vmdb::Plugins#versions #17755

Fryguy commented Jul 24, 2018 •

edited

Loading

Fryguy commented Jul 24, 2018

chessbyte commented Jul 24, 2018

Fryguy commented Jul 24, 2018 •

edited

Loading

bdunne commented Jul 24, 2018

Fryguy commented Jul 24, 2018

Fryguy commented Jul 24, 2018

Fryguy commented Aug 3, 2018

NickLaMuro left a comment

NickLaMuro Aug 3, 2018

Fryguy Aug 6, 2018

NickLaMuro Aug 6, 2018

Fryguy Aug 6, 2018

Fryguy Aug 6, 2018

NickLaMuro Aug 3, 2018

Fryguy Aug 6, 2018

NickLaMuro Aug 6, 2018

NickLaMuro Aug 3, 2018

Fryguy Aug 6, 2018

Fryguy Aug 6, 2018

NickLaMuro Aug 3, 2018

Fryguy Aug 6, 2018 •

edited

Loading

NickLaMuro Aug 3, 2018

Fryguy Aug 6, 2018

carbonin commented Aug 6, 2018 •

edited

Loading

simaishi commented Aug 6, 2018 •

edited

Loading

Fryguy commented Aug 6, 2018 •

edited

Loading

miq-bot commented Aug 6, 2018

Add Vmdb::Plugins#versions #17755

Add Vmdb::Plugins#versions #17755

Conversation

Fryguy commented Jul 24, 2018 • edited Loading

Fryguy commented Jul 24, 2018

chessbyte commented Jul 24, 2018

Fryguy commented Jul 24, 2018 • edited Loading

bdunne commented Jul 24, 2018

Fryguy commented Jul 24, 2018

Fryguy commented Jul 24, 2018

Fryguy commented Aug 3, 2018

NickLaMuro 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

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

Fryguy Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbonin commented Aug 6, 2018 • edited Loading

simaishi commented Aug 6, 2018 • edited Loading

Fryguy commented Aug 6, 2018 • edited Loading

miq-bot commented Aug 6, 2018

Fryguy commented Jul 24, 2018 •

edited

Loading

Fryguy commented Jul 24, 2018 •

edited

Loading

Fryguy Aug 6, 2018 •

edited

Loading

carbonin commented Aug 6, 2018 •

edited

Loading

simaishi commented Aug 6, 2018 •

edited

Loading

Fryguy commented Aug 6, 2018 •

edited

Loading