Skip to content
This repository has been archived by the owner on Nov 28, 2018. It is now read-only.

list directory contents if is directory #7

Merged
merged 1 commit into from Oct 20, 2014

Conversation

snobear
Copy link
Contributor

@snobear snobear commented Oct 3, 2014

I added a small feature to display a file listing if the given file is of type directory. This is useful for me and perhaps others, so here you go if you're interested!

@puppetcla
Copy link

Waiting for CLA signature by @snobear

@snobear - We require a Contributor License Agreement (CLA) for people who contribute to Puppet, but we have an easy click-through license with instructions, which is available at https://cla.puppetlabs.com/

Note: if your contribution is trivial and you think it may be exempt from the CLA, please post a short reply to this comment with details. http://docs.puppetlabs.com/community/trivial_patch_exemption.html

@puppetcla
Copy link

CLA signed by all contributors.

@richardc
Copy link
Contributor

This is interesting but could the behaviour be gated around an additional option which would default to false. Listing some directories can be expensive so it'd be better as a behaviour that the user has to opt-in to.

@snobear
Copy link
Contributor Author

snobear commented Oct 13, 2014

@richardc yes that's a good idea. it's expensive and the output could get verbose if there are a bunch of files in that directory.

How about an option called --dirlist which defaults to false?

@richardc
Copy link
Contributor

Seems like the correct switch, as it controls the dir_listing member being present.

snobear pushed a commit to snobear/mcollective-filemgr-agent that referenced this pull request Oct 13, 2014
@snobear
Copy link
Contributor Author

snobear commented Oct 14, 2014

Added a dirlist option, so you have to specify dirlist=true to show the listing for directories.

Question: Is there a way to completely hide the "Directory Listing" if its not applicable, i.e. if its just a single file and not a dir? A conditional display of "Directory Listing" if you will :).

If not, something like I have in place seems informative enough.

@richardc
Copy link
Contributor

Hide it from what/where?

@snobear
Copy link
Contributor Author

snobear commented Oct 14, 2014

Hide it from the output. If you're just checking the status of a file, i.e. a non-directory, it doesn't apply.

Anyway, scratch that. Here is what it currently does for file vs directory. Let me know your thoughts on where you think this feature stands.

File

mco rpc filemgr status file=/etc/puppet/puppet.conf dirlist=true -I test01

test01
          Access time: Tue Oct 14 02:01:39 -0400 2014
          Access time: 1413266499
          Change time: Mon Sep 15 10:10:39 -0400 2014
          Change time: 1410790239
    Directory Listing: nil (only applicable to directories)
                Group: 0
                  MD5: 1e50177294f00d8342e05a53e77a366a
                 Mode: 100644
    Modification time: Mon Sep 15 10:10:39 -0400 2014
    Modification time: 1410790239
                 Name: /etc/puppet/puppet.conf
               Status: present
              Present: 1
                 Size: 1171
                 type: file
                Owner: 0

Directory

mco rpc filemgr status file=/etc/puppet dirlist=true -I test01

test01
          Access time: Mon Oct 13 15:24:40 -0400 2014
          Access time: 1413228280
          Change time: Mon Oct 13 10:00:36 -0400 2014
          Change time: 1413208836
    Directory Listing: ["puppet.conf", "auth.conf", "modules"]
                Group: 0
                  MD5: 0
                 Mode: 40755
    Modification time: Mon Sep 15 16:34:33 -0400 2014
    Modification time: 1410813273
                 Name: /etc/puppet
               Status: present
              Present: 1
                 Size: 4096
                 type: directory
                Owner: 0

dir_filelist -= [".",".."]
reply[:dir_listing] = dir_filelist
else
reply[:dir_listing] = "nil (use dirlist=true option to display files in #{file})"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is optional, simply don't supply this when the user hasn't asked for it. That is to say lose the entire else condition.

snobear pushed a commit to snobear/mcollective-filemgr-agent that referenced this pull request Oct 15, 2014
@snobear
Copy link
Contributor Author

snobear commented Oct 15, 2014

Makes sense. I removed the else condition, and set the default value of the dirlist output to just nil.

@richardc
Copy link
Contributor

You shouldn't need a default if you just mark it as optional.

snobear pushed a commit to snobear/mcollective-filemgr-agent that referenced this pull request Oct 15, 2014
… value instead of just the presence of dirlist option
@snobear
Copy link
Contributor Author

snobear commented Oct 15, 2014

Gotcha. I removed the defaults and fixed some other small discrepancies.

@@ -95,7 +101,7 @@ action "status", :description => "Basic information about a file" do
:description => "File group",
:display_as => "Group"

output :type,
:description => "File type",
:display_as => "Type"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, are you sure you wanted to remove the specification for the :type field?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, thanks for catching that. Not sure how that happened. I'm out of town but will fix and push on Monday.=

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great - sorry for the slow turnaround, this week got away from me a bit.

snobear pushed a commit to snobear/mcollective-filemgr-agent that referenced this pull request Oct 20, 2014
@snobear
Copy link
Contributor Author

snobear commented Oct 20, 2014

@richardc fixed and ran some tests.

@richardc
Copy link
Contributor

This all looks good, could you squash it down into one commit and I'll merge.

@snobear
Copy link
Contributor Author

snobear commented Oct 20, 2014

Squashed. Let me know if you need anything else.

richardc added a commit that referenced this pull request Oct 20, 2014
list directory contents if is directory
@richardc richardc merged commit 61644d5 into choria-legacy:master Oct 20, 2014
@richardc
Copy link
Contributor

Merged. Thanks for the contribution.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants