Anonymous FTP upload #65

kbrock · 2018-09-24T16:57:06Z

This will allow customers to upload files to support sites like dropbox.redhat.com

To add this option, add to en.yml:

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

https://bugzilla.redhat.com/show_bug.cgi?id=1632433

NickLaMuro

This looks cool so far, and definitely along the lines of what I was considering. Most of the comments are more describing where I was differing how I envisioned it in my head, but are definitely open for debate and rejection based on personal preference and technical limitations.

Great start, and will be keeping an eye on this!

NickLaMuro · 2018-09-24T17:04:04Z

lib/manageiq/appliance_console/database_admin.rb

+
+        params = { :uri => uri }
+        params[:uri_username]     = "anonymous"
+        params[:uri_password]     = pass     if pass.present?


These can be left blank. Net::FTP will just assume "anonymous" when nil is passed.

NickLaMuro · 2018-09-24T17:05:19Z

spec/database_admin_spec.rb

-      let(:uri)               { File.dirname(subject.sample_url('s3')) }
-      let(:filename)          { File.basename(subject.sample_url('s3')) }
-      let(:example_uri)       { File.join(uri, filename) }
+      let(:example_uri)  { subject.send(:sample_url, 's3') }


You monster! You best be aligning those curly braces!!!1! 😡

NickLaMuro · 2018-09-24T17:13:14Z

lib/manageiq/appliance_console/database_admin.rb

+        validator    = ->(x) { x.to_s =~ /#{prompt_regex}/ } if prompt_regex
+        uri_filename = just_ask(prompt_text, nil, validator)
+        @uri         = uri.gsub(sample_case, uri_filename)
+        pass         = just_ask("email address (to use as the anonymous ftp password)")


Is this necessary for all cases? Can we make this as part of the config options?

This is the standard anonymous ftp thing. But I think skipping username/password makes things easier

NickLaMuro · 2018-09-24T17:16:34Z

lib/manageiq/appliance_console/database_admin.rb

+        require 'uri'
+        hostname     = URI(uri).host
+        prompt_text  = I18n.t("database_admin.prompt")[hostname.to_sym] || "Target filename (e.g.: #{sample_case})"
+        prompt_regex = I18n.t("database_admin.validator")[hostname.to_sym]


Curious if we could have different names for this, or they could be nested under a key so they are a little more contextual:

database_admin: custom_formats: ftp.example.com: filename_question: prompt: "The case number dash (-) filename. (e.g.: CASENUMBER-db.backup)" validator: "^[0-9]{4,}-..*" custom_questions: uri_password: prompt: "foo" validator: "blahblahblah" # I ran out of mojo partway through....

In addition, I always saw the "CASENUMER" as being a separate questions, and then we would munge the case number into the filename. Not sure how this would work with what you have here, so that might be more of a "nice to have" than anything, but thought I would bring it up.

Spoke with @jcarter12 and he felt this level of education was better handled by documentation.

I did take a stab at the different hierarchy

NickLaMuro · 2018-09-24T17:20:43Z

lib/manageiq/appliance_console/database_admin.rb

+          menu.choice(CANCEL) { |_| raise MiqSignalError }
+        end
+        if backup_type =~ %r{^ftps?://}
+          ask_anonymous_file_options(backup_type)


This regexp and method name seem very specific to FTP, but I could see this being used for protocols that aren't FTP. Could we possibly make this a little more generic? ask_custom_file_options or something (pick a better name... please 😩 )?

The next move is to localize these. But first step, get them out of prompts and into database_admin.rb

This allows the file types to be customized via localization downside, the name of the file_option types are now using send

carbonin

Looks pretty good to me. @kbrock can you provide a sample of what the anonymous case might look like in the I18n file? I understand we shouldn't edit the file in this repo, but I'm curious to see what it would look like now that I've seen #ask_custom_prompt

Lol read the damn description @carbonin

carbonin · 2018-09-26T18:17:03Z

lib/manageiq/appliance_console/database_admin.rb

-        when FTP_FILE   then ask_ftp_file_options
-        when CANCEL     then raise MiqSignalError
+        @backup_type = ask_with_menu(*file_menu_args) do |menu|
+          menu.choice(CANCEL) { |_| raise MiqSignalError }


I wonder if this is something we should just add to the end of every menu? Unlikely that it should be added to this PR, but this made me think of it.

@kbrock When I was typing this I was thinking of modifying ask_with_menu to add that as an option

Maybe just make an issue for this? I agree, that would definitely make this cleaner. Maybe also have it as an option to opt of ( something like ask_with_menu(*file_menu_args, :cancel_option => false) maybe?).

...which reminds me... I really need to add an issue for the "UI stuff" I have wanted to refactor in this...

carbonin · 2018-09-26T18:31:43Z

lib/manageiq/appliance_console/database_admin.rb

+      def file_options
+        @file_options ||= I18n.t("database_admin.menu_order").each_with_object({}) do |file_option, h|
+          # special anonymous ftp sites
+          if file_option =~ %r{^ftps?://([^/]*)/}


This might be easier to read without the regex. None of the other file options are uri formatted right? How about something like this:

if %w(ftp ftps).include?(URI(file_option).scheme)

Then we can also avoid the global match variable by using the host in the URI.

https://bugzilla.redhat.com/show_bug.cgi?id=1535345 https://bugzilla.redhat.com/show_bug.cgi?id=1632433 This will allow customers to upload files to support sites To add this option, add to `en.yml`: ```yml database_admin: menu_order: - local - ftp://ftp.example.com/incoming/999999-db.backup local: Local file prompts: ftp.example.com: filename_text: "The case number dash (-) filename. (e.g.: 12345-db.backup)" filename_validator: "^[0-9]{4,}-..*" ```

miq-bot · 2018-09-29T16:13:53Z

Checked commits kbrock/manageiq-appliance_console@463c79b~...51eb422 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 👍

miq-bot added the wip label Sep 24, 2018

NickLaMuro reviewed Sep 24, 2018

View reviewed changes

kbrock added 2 commits September 24, 2018 13:37

Move database admin sample_urls to database_admin

463c79b

The next move is to localize these. But first step, get them out of prompts and into database_admin.rb

localize upload target text and sample_urls

688f94a

This allows the file types to be customized via localization downside, the name of the file_option types are now using send

kbrock force-pushed the localized_file_locs branch 3 times, most recently from d6171f9 to 8bec73f Compare September 25, 2018 14:33

kbrock changed the title ~~[WIP] Anonymous FTP upload~~ Anonymous FTP upload Sep 25, 2018

miq-bot removed the wip label Sep 25, 2018

carbonin reviewed Sep 26, 2018

View reviewed changes

kbrock force-pushed the localized_file_locs branch 6 times, most recently from 24842e7 to 9c6a1e6 Compare September 28, 2018 23:14

kbrock force-pushed the localized_file_locs branch from 9c6a1e6 to 51eb422 Compare September 29, 2018 16:13

carbonin approved these changes Oct 1, 2018

View reviewed changes

carbonin self-assigned this Oct 1, 2018

carbonin added the bug label Oct 1, 2018

carbonin merged commit 6d4379d into ManageIQ:master Oct 1, 2018

carbonin added this to the Sprint 96 Ending Oct 8, 2018 milestone Oct 1, 2018

kbrock deleted the localized_file_locs branch October 1, 2018 14:37

This was referenced Oct 4, 2018

For database dumps don't modify the directory name ManageIQ/manageiq#18058

Merged

Don't set empty ENV values for database dumps ManageIQ/manageiq-gems-pending#378

Merged

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Anonymous FTP upload #65

Anonymous FTP upload #65

kbrock commented Sep 24, 2018 •

edited

Loading

NickLaMuro left a comment

NickLaMuro Sep 24, 2018

NickLaMuro Sep 24, 2018

NickLaMuro Sep 24, 2018

kbrock Sep 24, 2018

NickLaMuro Sep 24, 2018

NickLaMuro Sep 24, 2018

kbrock Sep 24, 2018

NickLaMuro Sep 24, 2018

carbonin left a comment •

edited

Loading

carbonin Sep 26, 2018

kbrock Sep 26, 2018

NickLaMuro Sep 28, 2018

carbonin Sep 26, 2018

miq-bot commented Sep 29, 2018

Anonymous FTP upload #65

Anonymous FTP upload #65

Conversation

kbrock commented Sep 24, 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

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

carbonin left a comment • 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

miq-bot commented Sep 29, 2018

kbrock commented Sep 24, 2018 •

edited

Loading

carbonin left a comment •

edited

Loading