Anonymous FTP request single file prompt (vs 2) #69

kbrock · 2018-10-04T16:45:59Z

We were prompting for a local data filename and a remote url.
But since this is streaming, it does not make sense.

This now:

properly populates the :remote_file_name value
hides/shows the prompt selectively for dump, backup, restore
has configurable rake options

related to:

configuration has been tweaked:

  database_admin:
    menu_order:
    - local
    - ftp://ftp.example.com/incoming/
    local: Local file
    prompts:
      ftp.example.com:
        enabled_for: ["backup", "dump"]
        filename_text: "The case number dash (-) filename. (e.g.: 12345-db.backup)"
        filename_validator: "^[0-9]{4,}-..*"
        rake_options:
          skip_directory: true

NickLaMuro

I will have to look over this more in a bit, but this should be enough to get you started.

NickLaMuro · 2018-10-04T17:01:37Z

lib/manageiq/appliance_console/database_admin.rb

-        uri_filename = ask_custom_prompt(hostname, 'filename', "Target filename (e.g.: #{sample_case})")
-        @uri         = server_uri.gsub(sample_case, uri_filename)
+        hostname  = URI(server_uri).host
+        @filename = ask_custom_prompt(hostname, 'filename', "Target filename for backup")


I realize this was added in the previous PR, and I wasn't really the reviewer, but why are we "passing" the default prompt if this method is only used in one spot?

I was reviewing a similar PR that @carbonin was (where he asked to axe some constants), but this seems like it makes more sense to just have this as a constant, or a frozen string in the method itself instead of passing it from here.

Also (not part of this line of code), but I think you can delete the constants at the top of this file for LOCAL_FILE, NFS_FILE, etc. since we don't use them any more (as of #65).

Separate commit/PR maybe? I think you would have a lot of tests that would need to be updated, for what it is worth.

ok, will make the prompt less generic.

Those constants are used all over the specs :( They have been simplified quite a bit (now just have 'ftp' vs the full text of the ftp string) -- we can hardcode phrases like "ftp" in the specs.

spec/database_admin_spec.rb

kbrock · 2018-10-04T18:56:17Z

ok

added the missing tests (they felt a little copy paste),
fixed custom_file prompt (removed the genericness)
removed the constants (that were only used in specs)

keep 'em comin'

NickLaMuro

Got a few things I need to look into yet (I think...), but a few more things I noticed.

NickLaMuro · 2018-10-04T19:31:47Z

lib/manageiq/appliance_console/database_admin.rb

@@ -38,6 +32,10 @@ def initialize(action = :restore, input = $stdin, output = $stdout)
        @task_params = []
      end

+      def local_backup?
+        backup_type == "local"


Pendantic: "local".freeze

But yes, I like this change. 👍

NickLaMuro · 2018-10-04T19:35:59Z

lib/manageiq/appliance_console/database_admin.rb

-        prompt_regex = prompts && prompts["#{prompt_name}_validator".to_sym]
+      def ask_custom_file_prompt(hostname)
+        # hostname has a period in it, so we need to look it up by [] instead of the traditional i18n method
+        prompts = I18n.t("database_admin.prompts", :default => nil).try(:[], hostname.to_sym)


Is the .try(:[], ...) necessary? I figured the .to_sym would handle and case you would be concerned about, and return a nil if nothing exists for it. Right?

I think you would have to change the :default to {}, but I think this would work after that:

prompts = I18n.t("database_admin.prompts", :default => {})[hostname.to_sym]

kbrock · 2018-10-15T13:25:40Z

@NickLaMuro thanks - got in the freeze and the :default => {} -- very nice touch with that one.

We were prompting for a local data filename and a remote url. But since this is streaming, it does not make sense. this now properly populates the :remote_file_name value

carbonin

Looks good to me. That spec file is getting hard to read, but too many tests is a good problem to have IMO 😄

@NickLaMuro anything more to add here?

kbrock · 2018-10-16T21:16:42Z

@NickLaMuro What I used to configure the appliance console in local development to act like the appliance vm and pickup my localization changes

$ pwd
/Users/kbrock/src/manageiq-appliance_console
$ cat ~/src/manageiq/productization/appliance_console/locales/appliance/en_productization.yml
---
en:
  database_admin:
    menu_order:
    - local
    - ftp://ftp.example.com/incoming
    local: Local file
    prompts:
      ftp.example.com:
        filename_text: "The case number dash (-) filename. (e.g.: 12345-db.backup)"
        filename_validator: "^[0-9]{4,}-..*"
$ bundle exec irb -I lib
irb> module ManageIQ; module ApplianceConsole; RAILS_ROOT = File.expand_path("../manageiq", Dir.pwd); end; end
# => "/Users/kbrock/src/manageiq"  
irb> require 'manageiq/appliance_console/i18n'
# => true  
irb> I18n.t("database_admin.prompts", :default => {})
# => {
  :"ftp.example.com"=>{
    :filename_text=>"The case number dash (-) filename. (e.g.: 12345-db.backup)",
    :filename_validator=>"^[0-9]{4,}-..*"
  }
}  
irb> I18n.load_path
# => [
  "/Users/kbrock/src/manageiq-appliance_console/locales/appliance/en.yml",
  "/Users/kbrock/src/manageiq/productization/appliance_console/locales/appliance/en_productization.yml"
]

NickLaMuro

I think these comments are best left for another PR, if at all, but tossing them out there anyway.

Feel free to merge without the changes.

NickLaMuro · 2018-10-17T20:02:13Z

lib/manageiq/appliance_console/database_admin.rb

-        params[:remote_file_name] = filename if filename
+        params = {
+          :uri              => uri,
+          :skip_directory   => true,


I am fine with this for now, but I think in the end, this should be part of the configuration, and not hard coded in.

Along with this, I really feel we should be hiding this option, or allowing it to be configured to do so, for the restore options since this is ultimately being implemented for an endpoint you won't be able to restore from.

Might need a follow up, but just my two cents.

kbrock · 2018-10-17T20:36:22Z

WIPping to find a more configurable way of adding flags for specific backup options

kbrock · 2018-10-17T21:07:31Z

Along with this, I really feel we should be hiding this option, or allowing it to be configured to do so, for the restore options since this is ultimately being implemented for an endpoint you won't be able to restore from.
-- @NickLaMuro

Further clarifying this comment:

For our current use case, this option should be displayed for backup and dump, but should be hidden for the restore menu.
There are 2 possible solutions so far for configuration:

the dry solution <== winner

@NickLaMuro suggests dry. It is no fun to have duplicate lists - duplication tends to go wrong quickly.

We display menu_order for all entries, unless an entry has enable_for key and the key does not include the current menu name (i.e.: dump, backup, restore).

---
  database_admin:
    menu_order:
    - local
    - ftp://ftp.example.com/incoming/
    local: Local file
    prompts:
      ftp.example.com:
        enabled_for: [dump, backup]  ###### NEW FEATURE
        filename_text: "The case number dash (-) filename. (e.g.: 12345-db.backup)"
        filename_validator: "^[0-9]{4,}-.+"

the less logic solution

@kbrock suggests duplicating but removing special parameters that negate the list. Duplication stinks, but conditional logic is worse.

If {restore, backup, dump}_menu_order is available, display that. Otherwise fall back to display menu_order.

---
  database_admin:
    menu_order:
    - local
    - ftp://ftp.example.com/incoming/
    restore_menu_order:    ###### NEW FEATURE
    - local
    - ftp://ftp.example.com/incoming/
    local: Local file
    prompts:
      ftp.example.com:
        filename_text: "The case number dash (-) filename. (e.g.: 12345-db.backup)"
        filename_validator: "^[0-9]{4,}-.+"

call it ask_custom_file_prompts and don't pretend that we are going to extend this

These constants were useful at a time, but are currently only used in tests. so they have been removed

some options are only available for restore vs backup This allows an options to be available for only one of them

kbrock · 2018-10-18T13:06:36Z

added rake_options and made custom endpoints enabled for various options (backup, dump, restore)

Does this work for you now @NickLaMuro ?

NickLaMuro

I think this looks good. Thanks for taking the extra time on this!

Comments left are minor, and not required... unless you want to be shamed for merging in a typo 😉

NickLaMuro · 2018-10-22T20:29:49Z

lib/manageiq/appliance_console/database_admin.rb

-        hostname     = URI(server_uri).host
-        uri_filename = ask_custom_prompt(hostname, 'filename', "Target filename (e.g.: #{sample_case})")
-        @uri         = server_uri.gsub(sample_case, uri_filename)
+        hostname  = URI(server_uri).host


This is minor:

I found this a bit confusing to me for a bit since I saw you parsing out the host from the server_uri, and thought that was being being assigned to @uri. Turns out that wasn't the case, but we also aren't using the hostname in any meaningful fashion except as a key. Seems like we could just as easily do this:

database_admin: menu_order: - local - ftp://ftp.example.com/incoming/ local: Local file prompts: ftp://ftp.example.com/incoming/: enabled_for: ["backup", "dump"] ...

And achieve the same affect by just passing around server_uri as the key.

Again, this is minor, just thought it was a bit confusing from both a code and configuration perspective since there is two different "keys" being passed around.

Lets keep this as the host for now.
Using the full path would simplify the code and may be more clear
But I like reading the configuration for the host version

Works for me.

If the above wasn't clear to on lookers, my above critique is pretty subjective, and I don't hold a hard opinion on this.

NickLaMuro · 2018-10-22T20:32:35Z

spec/database_admin_spec.rb

+        it "displays custom ftp option with enabled array" do
+          expect(I18n).to receive(:t).with("database_admin.menu_order").and_return(%w(local ftp://example.com/inbox/filename.txt))
+          expect(I18n).to receive(:t).with("database_admin.local").and_return("The Local file")
+          expect(I18n).to receive(:t).with("database_admin.prompts", :default => {}).at_least(:once).and_return(:"example.com" => { :enabled_for => %w(restore backup) })


Pedantic: Wouldn't this be a known quantity of .once or .twice? Would save some characters and would be more explicit with those since this shouldn't be a variable number of calls as far as I can tell.

Also, since this line is pretty long, could the prompt options be saved in a let(:prompt_options) to allow this to be more visable?

The actual value for enabled_for are different for most tests. I'll make a helper for this.

I'll make the twice more explicit

NickLaMuro · 2018-10-22T20:34:12Z

spec/database_admin_spec.rb

+
+        it "displays custom ftp option with enabled array" do
+          expect(I18n).to receive(:t).with("database_admin.menu_order").and_return(%w(local ftp://example.com/inbox/filename.txt))
+          expect(I18n).to receive(:t).with("database_admin.local").and_return("The Local file")


Pedantic: These above two seem like they could be tossed in a before hook to save them being defined twice.

NickLaMuro · 2018-10-22T20:34:47Z

spec/database_admin_spec.rb

+          PROMPT
+        end
+
+        it "hids custom ftp option with disabled string" do


Typo: s/hids/hides

NickLaMuro · 2018-10-22T20:48:53Z

spec/database_admin_spec.rb

      end

      it "cancels when CANCEL option is choosen" do
        say "6"
        expect { subject.ask_file_location }.to raise_error signal_error
      end
+
+      # this is the complete implementation. the other 2 are paired down version of this
+      context "with localized file upload" do


Pedantic: maybe "with I18n custom menu configs"?

If changed, make sure to change in the other two spots.

some prompts need custom options. --skip-directory=true is one option. We had it hardcoded before. this PR extracts it into its own configuration option

miq-bot · 2018-10-23T19:32:52Z

Checked commits kbrock/manageiq-appliance_console@e26f76d~...542178a 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. 🍪

kbrock · 2018-10-23T21:00:46Z

@miq-bot tag hammer/yes

miq-bot · 2018-10-23T21:01:18Z

@kbrock unrecognized command 'tag', ignoring...

Accepted commands are: add_label, add_reviewer, assign, close_issue, move_issue, remove_label, rm_label, set_milestone

carbonin · 2018-10-23T21:02:50Z

@kbrock The console is actually a released gem so at some point when the backup/dump work stabilizes, we'll cut a release and include that in the hammer branch of manageiq-appliance

NickLaMuro reviewed Oct 4, 2018

View reviewed changes

kbrock force-pushed the single_backup_prompt branch from efbac8b to d07b9f4 Compare October 4, 2018 18:54

NickLaMuro reviewed Oct 4, 2018

View reviewed changes

kbrock force-pushed the single_backup_prompt branch from d07b9f4 to 4dab261 Compare October 15, 2018 13:24

Anonymous FTP request single file prompt (vs 2)

e26f76d

We were prompting for a local data filename and a remote url. But since this is streaming, it does not make sense. this now properly populates the :remote_file_name value

kbrock force-pushed the single_backup_prompt branch from 4dab261 to 1710eb3 Compare October 15, 2018 15:35

carbonin approved these changes Oct 15, 2018

View reviewed changes

kbrock changed the title ~~Anonymous FTP request single file prompt (vs 2)~~ [WIP] Anonymous FTP request single file prompt (vs 2) Oct 16, 2018

miq-bot added the wip label Oct 16, 2018

kbrock changed the title ~~[WIP] Anonymous FTP request single file prompt (vs 2)~~ Anonymous FTP request single file prompt (vs 2) Oct 17, 2018

miq-bot removed the wip label Oct 17, 2018

NickLaMuro reviewed Oct 17, 2018

View reviewed changes

kbrock changed the title ~~Anonymous FTP request single file prompt (vs 2)~~ [WIP] Anonymous FTP request single file prompt (vs 2) Oct 17, 2018

miq-bot added the wip label Oct 17, 2018

kbrock added 2 commits October 18, 2018 07:13

Simplify ask_custom_prompt

a84f01b

call it ask_custom_file_prompts and don't pretend that we are going to extend this

Add missing tests for db backup custom prompts

d8163e2

kbrock force-pushed the single_backup_prompt branch from 1710eb3 to c8df718 Compare October 18, 2018 12:49

kbrock changed the title ~~[WIP] Anonymous FTP request single file prompt (vs 2)~~ Anonymous FTP request single file prompt (vs 2) Oct 18, 2018

miq-bot removed the wip label Oct 18, 2018

kbrock force-pushed the single_backup_prompt branch from c8df718 to 933c04e Compare October 18, 2018 12:54

kbrock added 2 commits October 18, 2018 09:05

remove unneeded {FTP,S3,NFS}_FILE constants

faf9d87

These constants were useful at a time, but are currently only used in tests. so they have been removed

provide enabled_for for prompts

4a7762b

some options are only available for restore vs backup This allows an options to be available for only one of them

kbrock force-pushed the single_backup_prompt branch from 933c04e to 28ca993 Compare October 18, 2018 13:05

NickLaMuro approved these changes Oct 22, 2018

View reviewed changes

introduce custom_prompt rake_options

67f2a59

some prompts need custom options. --skip-directory=true is one option. We had it hardcoded before. this PR extracts it into its own configuration option

kbrock force-pushed the single_backup_prompt branch 3 times, most recently from 738ba47 to b60b334 Compare October 23, 2018 18:55

trim down I18n text for custom menu/prompts

542178a

kbrock force-pushed the single_backup_prompt branch from b60b334 to 542178a Compare October 23, 2018 19:29

carbonin approved these changes Oct 23, 2018

View reviewed changes

carbonin self-assigned this Oct 23, 2018

carbonin added the enhancement label Oct 23, 2018

carbonin merged commit cd6f786 into ManageIQ:master Oct 23, 2018

carbonin added this to the Sprint 98 Ending Nov 5, 2018 milestone Oct 23, 2018

kbrock deleted the single_backup_prompt branch October 23, 2018 20:59

NickLaMuro mentioned this pull request Oct 29, 2018

Support for Swift Backup/Restore #66

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Anonymous FTP request single file prompt (vs 2) #69

Anonymous FTP request single file prompt (vs 2) #69

kbrock commented Oct 4, 2018 •

edited

Loading

NickLaMuro left a comment

NickLaMuro Oct 4, 2018

NickLaMuro Oct 4, 2018

kbrock Oct 4, 2018 •

edited

Loading

kbrock commented Oct 4, 2018

NickLaMuro left a comment

NickLaMuro Oct 4, 2018

NickLaMuro Oct 4, 2018

NickLaMuro Oct 4, 2018

NickLaMuro Oct 4, 2018

kbrock commented Oct 15, 2018

carbonin left a comment

kbrock commented Oct 16, 2018

NickLaMuro left a comment

NickLaMuro Oct 17, 2018

NickLaMuro Oct 17, 2018

kbrock commented Oct 17, 2018

kbrock commented Oct 17, 2018 •

edited

Loading

kbrock commented Oct 18, 2018

NickLaMuro left a comment

NickLaMuro Oct 22, 2018

kbrock Oct 23, 2018 •

edited

Loading

NickLaMuro Oct 23, 2018

NickLaMuro Oct 22, 2018

NickLaMuro Oct 22, 2018

kbrock Oct 23, 2018

NickLaMuro Oct 22, 2018

NickLaMuro Oct 22, 2018

NickLaMuro Oct 22, 2018

miq-bot commented Oct 23, 2018

kbrock commented Oct 23, 2018

miq-bot commented Oct 23, 2018

carbonin commented Oct 23, 2018

Anonymous FTP request single file prompt (vs 2) #69

Anonymous FTP request single file prompt (vs 2) #69

Conversation

kbrock commented Oct 4, 2018 • edited Loading

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

kbrock Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

kbrock commented Oct 4, 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

kbrock commented Oct 15, 2018

carbonin left a comment

Choose a reason for hiding this comment

kbrock commented Oct 16, 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

kbrock commented Oct 17, 2018

kbrock commented Oct 17, 2018 • edited Loading

the dry solution <== winner

the less logic solution

kbrock commented Oct 18, 2018

NickLaMuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock Oct 23, 2018 • 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

miq-bot commented Oct 23, 2018

kbrock commented Oct 23, 2018

miq-bot commented Oct 23, 2018

carbonin commented Oct 23, 2018

kbrock commented Oct 4, 2018 •

edited

Loading

kbrock Oct 4, 2018 •

edited

Loading

kbrock commented Oct 17, 2018 •

edited

Loading

kbrock Oct 23, 2018 •

edited

Loading