Adds MiqFtpLib #360

NickLaMuro · 2018-08-07T00:47:55Z

Helper module for common FTP interactions that will be shared in multiple parts of the manageiq codebase.

Lifted from FileDepotFtp, and will eventually replace some of the methods there (follow up PR).

Links

Originally extracted in [WIP] FileSplitter with FTP upload support manageiq#17549

NickLaMuro · 2018-08-07T00:53:13Z

@carbonin So this is just a portion of ManageIQ/manageiq#17549 that I eventually want to pull into this repo. I will eventually be making a MiqFtpUploadTarget class in this repo and some other classes to support what we discussed on Friday.

But putting this together now since it will require a follow up PR and was taking a bit of a break from main effort of getting a decent splitting interface in place around the MiqGenericMountSession and other upload targets. This will probably make more sense once that interface is more than a thought in my head (and an actual PR), but feel free to ask any questions you have about it now if needed and I can provide more context for you.

cc @jerryk55 @Fryguy

carbonin · 2018-08-07T12:49:39Z

lib/gems/pending/util/miq_ftp_lib.rb

+  def self.included(klass)
+    klass.send(:attr_accessor, :ftp)
+
+    klass.send(:attr_accessor, :uri) unless klass.instance_methods.include?(:uri=)


At first glance it feels like there should be a better way to check if there is already an attr_accessor for this. If nothing else maybe check for both the #uri= and #uri methods?

Yeah, I thought so too. Again, this is a straight rip off of what I did in ManageIQ/manageiq#17549 (simply changed and relocated the module), so I didn't change anything there.

Yeah, can we simplify this? Conditional attr_accessors feels complicated. I don't understand why it's conditional.

@jrafanie Sorry, missed this when looking at this earlier, but please look at #360 (comment) for clarity on to why this is the way. Specifically the first bullet point.

lib/gems/pending/util/miq_ftp_lib.rb

carbonin · 2018-08-07T19:31:04Z

Replying to #360 (comment)

Live by the 📖 die by the 📖 I guess ...

I am in the process of creating a MiqFileStorage and MiqFileStorage::Interface which is meant to be the base class for MiqGenericMountSession classes, and what I am currently going to call "MiqUploadTargets" (encompassing FTP, s3, Swift, etc.).

Is the plan here to replace MiqGenereicMountSession or are you creating an abstraction that MiqGenericMountSession will also inherit from? I think some kind of hierarchy diagram might be useful in this case.

MiqFileStorage::Interface will be in charge of defining top level methods and underlying implementation methods can be overwritten by subclasses per implementation (where the input from a IO stream goes when writing to a split file)

Why make the interface a separate module in the namespace? I think I would rather have the UploadTarget base class define the interface if that's what the existing (and future) classes will be inheriting from.

MiqFileStorage will be roughly what with_mount_session is at the moment in EvmDatabaseOps in ManageIQ/manageiq.

I'm not sure what this means. Will MiqFileStorage have a class method that instantiates an UploadTarget (previously mount session) and yield it? That sounds fine to me.

I plan on putting FTP and s3 integrations under a UploadTarget (working title), which will be managed by MiqFileStorage, but conform to the MiqFileStorage::Interface

Okay, so MiqFileStorage will create instances of UploadTargets? Is the plan to do something similar to what MiqGenericMountSession.new_session does now? That sounds good, but see my previous comment about the interface living in the UploadTarget base class rather than in a module of MiqFileStorage

the FtpUploadTarget will want to probably share the connection logic of FileDepotFtp, and I see this as the easiest way to do it.

Makes sense to me. I was just trying to simplify the logging bit.

So as I understand it the design (which is really what this conversation ended up being) sounds like this.

UploadTarget: Base class of current MiqGenericMountSessions as well as new non-mountable storage things.
MiqFileStorage: Factory of UploadTargets which sounds like it will be a mashup of the current MiqGenericMountSession.new_session method and EvmDatabaseOps.with_mount_session
FtpUploadTarget: subclass of UploadTarget, but not MiqGenericMountSession

This all sounds good to me. The only changes/refinements I would suggest are pushing the new_session factory type logic into UploadTarget.new (maybe adopting the null object pattern if it makes sense, I'm thinking something like the way we implemented pdf generation or network interfaces) and maybe spelling out the interface you have planned for UploadTarget before doing all the work.

@NickLaMuro @Fryguy If you got this far does this make sense? Should we put this conversation somewhere else?

NickLaMuro · 2018-08-07T21:59:36Z

@NickLaMuro @Fryguy If you got this far does this make sense? Should we put this conversation somewhere else?

For those following along at home, the answer was "yes", and "we did". Most of what we (@carbonin and myself) discussed out band should answer the questions above in the new PR (when I get something concrete to share), so I will save the summary for then, and try to remember to link back to here later (if I remember).

NickLaMuro · 2018-08-29T20:36:32Z

@carbonin Do you have any reservations about what I am doing here still? I realize that your comment from before and the other one (I marked that one as resolved just because it got off topic a bit) are somewhat still as they were.

#368 , however, now can provide some clarity (built off of #361 currently), so if there was any curiosity as to how this would be used, that would be the intended implementation.

But to the comments you had previously (don't read if you are just going to merge anyway... or hate 📚):

Adds MiqFtpLib #360 (comment) : In FileDepotFtp (well, basically FileDepot), uri comes from ActiveRecord as it is a field in the DB, so that check is necessary (I do not want to override ActiveRecord's implementation if I can avoid it). The other option would be to not have this module implement uri as an accessor in included, and leave that to the including class to do. We could even raise an error for good measure to make sure they conform to the mixin's interface.
Adds MiqFtpLib #360 (comment) : I don't really have a good solution for this. What was there was carry over from the logging from FileDepotFtp (which at least one person must have found useful), and that is all cobbled together from the coupled, but disconnected Vmdb::Logger and Vmdb::Logging paradigm that we have plastered throughout the ManageIQ ecosystem (a source of constant frustration for me). I feel like that is a long winded refactoring issue that I don't want to address now, but is why that portion of this code smells so bad... 💩.

Will be used for spinning up a local FTP server in the tests for specs that require one.

Context that make sure to spin up an FTP server when included, and will make some helper methods available. Uses the `ftpd` gem to handle the ftp server.

carbonin · 2018-09-06T14:02:58Z

lib/gems/pending/util/miq_ftp_lib.rb

+# logging if not already done for the particular class (that follows the
+# VmdbLogger conventions, and setup a `uri` attr_accessor if that doesn't
+# already exist.
+module MiqFtpLib


I still think we should make this a class around FTP interaction with its own logger. Then we also don't have to be responsible for what methods are defined on the model (or other thing) using this class.

@carbonin is this in response to this:

#360 (comment)

or a comment on it's own?

I think it's kind of both. I read that response a few days ago then re-read the whole mixin here and I'm still not comfortable with this being an included module.

I think there would be more value and stability in making this a standalone class than trying to fit this in as a mixin. The refactoring you would have to do in FileDepotFtp seems minimal and I think it would make this class much more stable and readable.

TLDR;
Make it a class, take a uri as a parameter to initialize, and give it a #logger= method with a null logger as a default. That should take care of all of the meta-programming in .included here.

I think there would be more value and stability...

Sure, maybe long term. But as a reminder, this came about from your original comment here:

ManageIQ/manageiq#17549 (comment)

In which I was trying to meet halfway in regards to making this DRY. But, to your comment that:

The refactoring you would have to do in FileDepotFtp seems minimal...

I disagree entirely. These methods are EXACTLY as they are in FileDepotFtp, meaning there is next to zero risk on me doing what I originally had done in there as part of ManageIQ/manageiq#17549 which was gut these methods from FileDepotFTP and replace them via include MiqFtpLib (previously here).

With this suggestion, it is a re-write of FileDepotFtp effectively, because half of the methods I am not using here still have their own _log method that I would need to handle (or accept disparaging loggers for).

Also, @ftp is no longer part of the class, as it would instead be a reference to this new class, and so everything that isn't a part of MiqFtpLib as it is now that isn't removed from FileDepotFtp basically going to be re-written as a result. The "specs" for FileDepotFtp (which is about 90% mocking, and 10% actually testing things), would also probably completely need to be re-written as a result.

Not to mention basically re-working the main parts of ManageIQ/manageiq-gems-pending#368 as a result.

tl; dr No, this would not be easier to write this as a class. Unless you can convince "the powers that be" that this is worth said re-write that I didn't estimate for, I disagree with the suggestion.

Again [ref], if the meta-programming is an issue, I can basically remove most of the included block (if not all of it), and just assume this is an "interface" module, and those methods inside of included can then required by the "including" class. This goes for both the uri bit and the _log, and would have zero impact on FileDepotFtp, and have a minor amount of impact on #368 .

Okay, I prefer the method you mentioned about this being an "interface" module. The metaprogramming here just makes me a bit too uncomfortable, so if you can design this in a way that does less (or none) of that, I'm 👍

Done. I kept attr_accessor :ftp in since it has next to no meta-programming, but still tries to reach peak minimum LOC added, to appease @jrafanie ...

Edit: technically... I think it adds one more by doing this way... but whatever...

jrafanie · 2018-09-06T14:17:44Z

spec/util/miq_ftp_lib_spec.rb

+    context "when the class already has a uri=" do
+      subject { OtherFTPKlass.new }
+
+      it "has a `uri` accessor" do


yikes backticks...

jrafanie · 2018-09-06T14:18:12Z

spec/util/miq_ftp_lib_spec.rb

+      expect(subject.ftp).to eq ftp_instance
+    end
+
+    it "has a `uri` accessor" do


single quote instead of backtick?

jrafanie · 2018-09-06T14:24:51Z

lib/gems/pending/util/miq_ftp_lib.rb

+
+# Helper methods for net/ftp based classes and files.
+#
+# Will setup a `@ftp` attr_accessor to be used as the return value for


Does rdoc treat backticks in comments differently than single quotes? I'm not sure why there's backticks in these comments and it seems like there's a good reason.

I am treating backticks (here, and in your other comments) as you would in markdown to represent a portion of code with that is being talked about inside of the comment/description (variables or methods that are contextually relevant).

Backticks inside a string have no execution, and are simply there for emphasis for specific identifiers, using a standard syntax I assume most would be familiar with.

yard accepts markdown (and thus backticks) for comments, I believe

@Fryguy Yes, and so does just vanilla RDoc shipped with Ruby:

http://ruby-doc.org/stdlib-2.4.1/libdoc/rdoc/rdoc/RDoc/Markup.html#class-RDoc::Markup-label-Supported+Formats

carbonin · 2018-09-07T20:20:15Z

lib/gems/pending/util/miq_ftp_lib.rb

+# Will setup a `@ftp` attr_accessor to be used as the return value for
+# `.connect`, the main method being provided in this class.  Will also setup
+# logging if not already done for the particular class (that follows the
+# VmdbLogger conventions, and setup a `uri` attr_accessor if that doesn't


I think this is out of date now, right?

Yup! Good catch.

carbonin · 2018-09-07T20:21:25Z

lib/gems/pending/util/miq_ftp_lib.rb

@@ -0,0 +1,76 @@
+require 'net/ftp'
+require 'logger'


I don't think we need this anymore as were just calling a _log method which should have been defined on the including class.

This is lifted from the FileDepotFtp code in the manageiq repo, and is just some shared methods around connecting to an FTP endpoint. Adds some tests around it as well.

miq-bot · 2018-09-07T21:08:14Z

Some comments on commits NickLaMuro/manageiq-gems-pending@a1ac64c~...25baa0b

spec/support/contexts/with_ftp_server.rb

⚠️ - 15 - Detected puts. Remove all debugging statements.
⚠️ - 53 - Detected puts. Remove all debugging statements.

miq-bot · 2018-09-07T21:08:17Z

Checked commits NickLaMuro/manageiq-gems-pending@a1ac64c~...25baa0b with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🏆

NickLaMuro · 2018-09-07T21:18:22Z

@carbonin Just an FYI about how I "addressed" this comment:

I am still requiring 'logger' in the specs in the off chance it hasn't been included by something else. In classes that include this, I will also be including it, again, just as a safety measure.

Example in a later PR: NickLaMuro/manageiq-gems-pending@ftp_object_storage~2...ftp_object_storage#diff-4cf23bcfce6b36adcd0045ffbd1b724e

(EDIT: today I learned I can use [branch]~2..[branch] in github's compare view)

carbonin · 2018-09-10T13:17:15Z

Ehh, I'm not sure you can 😆

NickLaMuro · 2018-09-10T14:47:24Z

Ehh, I'm not sure you can 😆

@carbonin Yeah, looks like GHFM (GitHub Flavored Markdown) f'd with their own link:

https://github.com/NickLaMuro/manageiq-gems-pending/compare/ftp_object_storage~2...ftp_object_storage#diff-4cf23bcfce6b36adcd0045ffbd1b724e

That DOES work, but got fudged when it tried parsing out the # part in it when I left it as a wrong link in the last message...

... so I guess TIL on that as well...

NickLaMuro assigned carbonin Aug 7, 2018

NickLaMuro requested a review from carbonin August 7, 2018 00:48

carbonin reviewed Aug 7, 2018

View reviewed changes

NickLaMuro force-pushed the miq_ftp_lib branch 4 times, most recently from 7201948 to 0184546 Compare August 22, 2018 21:34

NickLaMuro mentioned this pull request Aug 29, 2018

Add MiqFtpStorage #368

Merged

3 tasks

NickLaMuro force-pushed the miq_ftp_lib branch 4 times, most recently from baf329a to 1cc26ac Compare August 29, 2018 19:47

NickLaMuro added 2 commits August 30, 2018 15:39

Add ftpd as a gem dependency

a1ac64c

Will be used for spinning up a local FTP server in the tests for specs that require one.

Add with_ftp_server rspec shared_context

04552b0

Context that make sure to spin up an FTP server when included, and will make some helper methods available. Uses the `ftpd` gem to handle the ftp server.

NickLaMuro force-pushed the miq_ftp_lib branch from 1cc26ac to 965cc70 Compare August 30, 2018 20:39

carbonin reviewed Sep 6, 2018

View reviewed changes

jrafanie reviewed Sep 6, 2018

View reviewed changes

NickLaMuro force-pushed the miq_ftp_lib branch from 965cc70 to afa4832 Compare September 7, 2018 18:54

carbonin reviewed Sep 7, 2018

View reviewed changes

Adds MiqFtpLib

25baa0b

This is lifted from the FileDepotFtp code in the manageiq repo, and is just some shared methods around connecting to an FTP endpoint. Adds some tests around it as well.

NickLaMuro force-pushed the miq_ftp_lib branch from afa4832 to 25baa0b Compare September 7, 2018 21:07

carbonin approved these changes Sep 10, 2018

View reviewed changes

carbonin added enhancement gaprindashvili/no labels Sep 10, 2018

carbonin merged commit b6ca5f0 into ManageIQ:master Sep 10, 2018

carbonin added this to the Sprint 94 Ending Sept 10, 2018 milestone Sep 10, 2018

NickLaMuro mentioned this pull request Sep 19, 2018

MiqFileStorage interface and subclassing (with file splitting) #361

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds MiqFtpLib #360

Adds MiqFtpLib #360

NickLaMuro commented Aug 7, 2018

NickLaMuro commented Aug 7, 2018

carbonin Aug 7, 2018

NickLaMuro Aug 7, 2018

jrafanie Sep 6, 2018 •

edited

Loading

NickLaMuro Sep 6, 2018

carbonin commented Aug 7, 2018 •

edited by Fryguy

Loading

NickLaMuro commented Aug 7, 2018

NickLaMuro commented Aug 29, 2018 •

edited

Loading

carbonin Sep 6, 2018

NickLaMuro Sep 6, 2018

carbonin Sep 6, 2018

NickLaMuro Sep 6, 2018 •

edited

Loading

carbonin Sep 7, 2018

NickLaMuro Sep 7, 2018 •

edited

Loading

jrafanie Sep 6, 2018

jrafanie Sep 6, 2018

jrafanie Sep 6, 2018

NickLaMuro Sep 6, 2018 •

edited

Loading

Fryguy Sep 7, 2018

NickLaMuro Sep 7, 2018

carbonin Sep 7, 2018

NickLaMuro Sep 7, 2018

carbonin Sep 7, 2018

NickLaMuro Sep 7, 2018

miq-bot commented Sep 7, 2018

miq-bot commented Sep 7, 2018

NickLaMuro commented Sep 7, 2018 •

edited

Loading

carbonin commented Sep 10, 2018

NickLaMuro commented Sep 10, 2018 •

edited

Loading

Adds MiqFtpLib #360

Adds MiqFtpLib #360

Conversation

NickLaMuro commented Aug 7, 2018

Links

NickLaMuro commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbonin commented Aug 7, 2018 • edited by Fryguy Loading

NickLaMuro commented Aug 7, 2018

NickLaMuro commented Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro Sep 7, 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

NickLaMuro Sep 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

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 7, 2018

miq-bot commented Sep 7, 2018

NickLaMuro commented Sep 7, 2018 • edited Loading

carbonin commented Sep 10, 2018

NickLaMuro commented Sep 10, 2018 • edited Loading

jrafanie Sep 6, 2018 •

edited

Loading

carbonin commented Aug 7, 2018 •

edited by Fryguy

Loading

NickLaMuro commented Aug 29, 2018 •

edited

Loading

NickLaMuro Sep 6, 2018 •

edited

Loading

NickLaMuro Sep 7, 2018 •

edited

Loading

NickLaMuro Sep 6, 2018 •

edited

Loading

NickLaMuro commented Sep 7, 2018 •

edited

Loading

NickLaMuro commented Sep 10, 2018 •

edited

Loading