Add default manifest and API controller for loading it #8

mturley · 2019-08-06T20:25:01Z

This PR adds a copy of the v1.0.0 manifest at config/default-manifest.json, and adds a new API endpoint at /api/migration_analytics_manifest for fetching it in the UI.

The API call will:

Check to see if there is a user-provided manifest at /tmp/migration-analytics-manifest.json on the appliance, and use that manifest if it is present and contains valid JSON.
If no valid tmp_manifest is present, it will instead use the default manifest stored in the gem.
Return an object with the following properties:
- tmp_manifest_present (boolean) - whether or not there is a file at /tmp/migration-analytics-manifest.json
- tmp_manifest_valid (boolean) - whether or not there is a valid JSON file at /tmp/migration-analytics-manifest.json
- path (string) - the path to the manifest JSON file being used
- body (object) - the parsed contents of the manifest JSON

We can use this to load a manifest into the UI, and:

Display the current version of that manifest
Indicate whether the user's manually provided manifest is being used or is invalid
Pass it to the data collector API (still to be developed).

mturley · 2019-08-06T20:43:31Z

@Fryguy now that I'm adding some real Ruby code to this repo, we'll need to set up Ruby unit testing stuff and fix the setup/update scripts (#2), which I could use some help with.

Also, based on some discussions with @agrare, I'm not sure exactly how we should move forward with the API for running the data collector and downloading the file. We'll need to:

Add an API endpoint that takes the manifest and selected providers, and calls Cfme::CloudServices::InventorySync::bundle_queue
Add some way of exposing the output .tar.gz file from that task as a URL the user's browser can download from (this is tricky, I have no idea where to start... @agrare suggested we could specify a server_guid when calling bundle_queue to make sure the file ends up on the same server as the API, and then make sure we're specifying a destination path that is within the apache root, and using that context to produce a URL?)
Add some API request the user can make to determine that payload download URL based on the task object
Add some mechanism for cleaning up the old .tar.gz files, maybe as a scheduled task? I also have no idea where to start here.

Are you guys (@Fryguy @agrare) still planning to develop all of the above, or do we need to get some help from @djberg96 and others? I know they are busy with Warm Migration for IMS, and I know you guys are busy too. But I'm a little worried that by offering to help take a stab at some of this stuff I gave the impression that I had it all covered, and if we do really need to get this done for the code freeze on Monday, I don't think I'll be able to build it on time myself considering I'm a JS guy learning Ruby/Rails as I go.

Thanks for all the help so far.

cc @bthurber

cfme-migration_analytics.gemspec

app/controllers/api/migration_analytics_manifest_controller.rb

agrare · 2019-08-07T12:22:24Z

app/controllers/api/migration_analytics_manifest_controller.rb

+    def index
+      tmp_manifest_path = '/tmp/migration-analytics-manifest.json'
+      tmp_manifest = File.file?(tmp_manifest_path) ? parse_json_if_valid(File.read(tmp_manifest_path)) : nil
+      tmp_manifest_valid = tmp_manifest != nil && tmp_manifest != :invalid


Maybe we take this one step at a time and just return the builtin manifest for now, then expand to handle user uploaded also

I can break this into two PRs if you want. But I know Miguel was asking for us to make sure there is always a way for the user to manually upgrade the included manifest, in case we ship this feature and then release some change to the cloud.redhat.com part of Migration Analytics that depends on a newer manifest that hasn't been released built-in yet.

Maybe he'll have an opinion here.. cc @mperezco

I don't like having it in /tmp because some systems clear that on reboot and on a schedule as a cron task, but I don't want to hold this PR up while we discuss where the user could upload it.

agrare · 2019-08-07T12:22:49Z

app/controllers/api/migration_analytics_manifest_controller.rb

+        manifest = tmp_manifest
+      else
+        manifest_path = File.join(
+          Bundler.environment.specs['cfme-migration_analytics'].first.full_gem_path,


Think you can just do Cfme::MigrationAnalytics::Engine.root.join("config", "default-manifest.json")

Ah, that is better, thanks.

Also, I notice that examples from you and @Fryguy all use double-quotes and I've been using single-quotes in all my Ruby so far, just because that's what we did in v2v. Do you guys have an opinion on which should be used? I can switch them all over to double-quotes.

That's mostly a personal preference, in ruby you can't do string interpolation in a string with single quotes which some people think is a good thing (i.e. it indicates the string is a constant).

Personally I think it looks sloppy having a mixture of '' and "". You have to use "" if you do any string interpolation, so I tend to just use "" everywhere.

Sounds good. I agree it's sloppy to have both. Changed them all to "".

I pushed this commit too late to be merged in this PR, so I added it to #10.

app/controllers/api/migration_analytics_manifest_controller.rb

agrare · 2019-08-07T12:24:38Z

@abellotti can you help review this from the API side?

agrare · 2019-08-07T12:44:41Z

@mturley I tested with my local changes and I had to re-enable vmdb_plugin? in the engine here to get the manageiq-api to mount this plugin's API, after that it worked great

abellotti · 2019-08-07T15:18:47Z

app/controllers/api/migration_analytics_manifest_controller.rb

+
+      res = {
+        :tmp_manifest_present => tmp_manifest != nil,
+        :tmp_manifest_valid => tmp_manifest_valid,


if this is part of an API response, are these the right key names we want ? tmp_... what if it's in a different location ? I would expect just "manifest_present" and "manifest_valid".

My goal here was to have the UI be able to tell if the manifest being returned is the one built-in on the appliance (the default-manifest.json file being added here) or a file they included in the /tmp/ directory, and whether the latter is being ignored because it's invalid JSON. Maybe it should be "override_manifest_present" and "override_manifest_valid" instead?

Removed this feature for now per @agrare 's request.

config/api.yml

abellotti · 2019-08-07T15:33:44Z

app/controllers/api/migration_analytics_manifest_controller.rb

+      render_resource :migration_analytics_manifest, res
+    end
+  end
+end


we need specs added for the API. so spec/requests/migration_analytics_manifest_spec.rb, maybe look at https://github.com/RedHatCloudForms/cfme-cloud_services/pull/23/files for spec/ files for the api file plus the spec_helper.rb and support/api_request_helpers.rb.

Ok, I'll take a look and try to get these started. I got stuck trying to get the Travis config's Ruby checks to work in this repo though, I may need some help figuring that out (the bin/setup script fails, probably because it was depending on something from https://github.com/martinpovolny/miq_plugin_example that I removed). I have an issue to track that: #2

…t_feature

mturley · 2019-08-07T17:59:05Z

@agrare we have vmdb_plugin? disabled here just because @bthurber asked for the UI to be hidden in current builds. Do you think I should re-enable it in this PR? It would certainly make it easier to test these changes on appliance builds, but it would mean the UI can be reached if you know the URL (I think?). That's probably fine, I assume, but I'll check with others.

miq-bot · 2019-08-07T17:59:37Z

Some comments on commits mturley/cfme-migration_analytics@7e2d7e7~...1c9d3e2

app/controllers/api/migration_analytics_manifest_controller.rb

💣 💥 🔥 🚒 - 13 - Detected cfme

miq-bot · 2019-08-07T17:59:40Z

Checked commits mturley/cfme-migration_analytics@7e2d7e7~...1c9d3e2 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 2 offenses detected

**

💣 💥 🔥 🚒 - Linter/Rubocop - missing config files
💣 💥 🔥 🚒 - Linter/Yaml - missing config files

bthurber · 2019-08-07T21:51:48Z

@agrare we have vmdb_plugin? disabled here just because @bthurber asked for the UI to be hidden in current builds. Do you think I should re-enable it in this PR? It would certainly make it easier to test these changes on appliance builds, but it would mean the UI can be reached if you know the URL (I think?). That's probably fine, I assume, but I'll check with others.

The only requirement we have is that MA isn't visible as an option under Migration in the UI with the ability to add it back to the menu if needed. Similar to what we did for IMS 1.0 beta.

agrare · 2019-08-08T12:27:03Z

@agrare we have vmdb_plugin? disabled here just because @bthurber asked for the UI to be hidden in current builds. Do you think I should re-enable it in this PR? It would certainly make it easier to test these changes on appliance builds, but it would mean the UI can be reached if you know the URL (I think?). That's probably fine, I assume, but I'll check with others.

The only requirement we have is that MA isn't visible as an option under Migration in the UI with the ability to add it back to the menu if needed. Similar to what we did for IMS 1.0 beta.

Okay thanks @bthurber, I'm fine leaving vmdb_plugin? false for now but we'll need to turn it on for the API to work. Without the UI the API isn't really helpful though anyway so its really just an issue for testing/development.

agrare

This is a good starting point, thanks @mturley !

Add default manifest and API controller for loading it

Add default manifest and API controller for loading it (cherry picked from commit 3847740)

simaishi · 2019-08-08T22:03:15Z

Ivanchuk backport details:

$ git log -1
commit 3b1e74f9e81a9812ce0358618b66be8bbcfe1194
Author: Adam Grare <[email protected]>
Date:   Thu Aug 8 11:57:10 2019 -0400

    Merge pull request #8 from mturley/manifest-api-endpoint
    
    Add default manifest and API controller for loading it
    
    (cherry picked from commit 3847740a36fa5bb0a4fcad174160d1419165cb7c)

mturley added 3 commits August 6, 2019 13:44

Add hello-world API controller

7e2d7e7

Add initial default manifest file

4c77dbb

/api/migration_analytics_manifest loads tmp if present, or default

2d7550d

mturley added enhancement New feature or request changelog/no ivanchuk/yes labels Aug 6, 2019

mturley requested review from Fryguy, mzazrivec and agrare August 6, 2019 20:25

agrare reviewed Aug 7, 2019

View reviewed changes

cfme-migration_analytics.gemspec Outdated Show resolved Hide resolved

agrare reviewed Aug 7, 2019

View reviewed changes

app/controllers/api/migration_analytics_manifest_controller.rb Outdated Show resolved Hide resolved

agrare reviewed Aug 7, 2019

View reviewed changes

app/controllers/api/migration_analytics_manifest_controller.rb Outdated Show resolved Hide resolved

agrare reviewed Aug 7, 2019

View reviewed changes

app/controllers/api/migration_analytics_manifest_controller.rb Outdated Show resolved Hide resolved

agrare reviewed Aug 7, 2019

View reviewed changes

app/controllers/api/migration_analytics_manifest_controller.rb Outdated Show resolved Hide resolved

agrare reviewed Aug 7, 2019

View reviewed changes

app/controllers/api/migration_analytics_manifest_controller.rb Outdated Show resolved Hide resolved

abellotti reviewed Aug 7, 2019

View reviewed changes

mturley added 2 commits August 7, 2019 13:47

Clean up controller code

93803de

Define collection_actions using the red_hat_cloud_services miq_produc…

1c9d3e2

…t_feature

Remove /tmp manifest upload feature

9ed71c3

agrare self-assigned this Aug 8, 2019

agrare approved these changes Aug 8, 2019

View reviewed changes

agrare merged commit 9ed71c3 into RedHatCloudForms:master Aug 8, 2019

agrare added a commit that referenced this pull request Aug 8, 2019

Merge pull request #8 from mturley/manifest-api-endpoint

3847740

Add default manifest and API controller for loading it

This was referenced Aug 8, 2019

Add new red_hat_migration_analytics product feature #10

Merged

Travis CI is not configured for Ruby checks because the bin/setup script fails #2

Open

simaishi pushed a commit that referenced this pull request Aug 8, 2019

Merge pull request #8 from mturley/manifest-api-endpoint

3b1e74f

Add default manifest and API controller for loading it (cherry picked from commit 3847740)

simaishi added ivanchuk/backported and removed ivanchuk/yes labels Aug 8, 2019

agrare added this to the Sprint 118 Ending Aug 19, 2019 milestone Aug 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add default manifest and API controller for loading it #8

Add default manifest and API controller for loading it #8

mturley commented Aug 6, 2019

mturley commented Aug 6, 2019 •

edited

Loading

agrare Aug 7, 2019

mturley Aug 7, 2019

agrare Aug 7, 2019

mturley Aug 8, 2019

agrare Aug 7, 2019

mturley Aug 7, 2019

agrare Aug 7, 2019

mturley Aug 8, 2019

mturley Aug 8, 2019

agrare commented Aug 7, 2019

agrare commented Aug 7, 2019

abellotti Aug 7, 2019

mturley Aug 7, 2019

mturley Aug 8, 2019

abellotti Aug 7, 2019

mturley Aug 7, 2019

mturley commented Aug 7, 2019 •

edited

Loading

miq-bot commented Aug 7, 2019

miq-bot commented Aug 7, 2019 •

edited by mturley

Loading

bthurber commented Aug 7, 2019

agrare commented Aug 8, 2019

agrare left a comment

simaishi commented Aug 8, 2019

Add default manifest and API controller for loading it #8

Add default manifest and API controller for loading it #8

Conversation

mturley commented Aug 6, 2019

mturley commented Aug 6, 2019 • edited Loading

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

agrare commented Aug 7, 2019

agrare commented Aug 7, 2019

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

mturley commented Aug 7, 2019 • edited Loading

miq-bot commented Aug 7, 2019

miq-bot commented Aug 7, 2019 • edited by mturley Loading

bthurber commented Aug 7, 2019

agrare commented Aug 8, 2019

agrare left a comment

Choose a reason for hiding this comment

simaishi commented Aug 8, 2019

mturley commented Aug 6, 2019 •

edited

Loading

mturley commented Aug 7, 2019 •

edited

Loading

miq-bot commented Aug 7, 2019 •

edited by mturley

Loading