Option for create nfs exported directory #186

bschonec · 2024-05-20T15:22:47Z

I have a case where an application I'm installing expects that the folders to be exported via NFS do NOT exist at application install time.

This PR allows for the edge case where you don't want to create the directories but you do want to manage the NFS export file(s).

tuxmea · 2024-06-10T08:54:32Z

@bschonec Can you please rebase your branch?

bschonec · 2024-06-10T12:11:33Z

@Tux, sure thing. I'm not sure where I should put the tests for the directory (not) creation. There are a lot of places in the tests where it does "ensure directory." I only need to test for "not ensure directory." Should I just create a simple nfsv4/nfsv3 section and do a "not ensure directory" and not test for anything else?

h-haaks · 2024-06-10T12:27:43Z

manifests/functions/create_export.pp

-        mode                    => $mode,
-        selinux_ignore_defaults => true,
+      if $create_dir {
+        filepath { $name:


Why the switch of file into filepath? That's not relevant when it comes to creating the folder or not and there is another PR for this change.

I would argue that this module almost never should create this folder as that normally is the responsibility of another module. I think it would be even worse to let it create an entire path ...

It would be a breaking change, but I would consider setting $create_dir default to false.

Please stay with file for now. Besides this: there are two implementations for creating recursive directories: filepath and dirtrtee. We should have this change in a separate PR so we can check and review which module is the better one to use.

And the module it self has this https://github.com/voxpupuli/puppet-nfs/blob/master/manifests/functions/mkdir.pp :)

tuxmea · 2024-06-11T16:17:19Z

manifests/server/export.pp

@@ -57,7 +57,9 @@
 # @param v4_export_name
 # @param nfsv4_bindmount_enable
 #
-# @examples


please re-add the @examples strings tag.

Done. (I think). @tuxmea, can you explain or point me to an explanation of what @examples does?

I think it should be @example, and it's is a tag used by puppet strings to render an example in REFERENCE.md
See https://www.puppet.com/docs/puppet/8/puppet_strings.html

It should be moved below the last @param at line 65

Should I create a separate pull request of off the current master to fix @examples/@example?

If so, #193

No need to have another PR. please fix it here and please keep in mind that the code example must be indented by 2 spaces.

I've fixed it here but I have no idea what you're referring to when you say, "...code example must be indented by 2 spaces."

@bschonec the example should have a description, then the example code. The code should be indented two spaces, so there should be three spaces after the hash mark #. Examples: https://github.com/search?q=repo%3Avoxpupuli%2Fpuppet-systemd%20%40example&type=code

Tag was readded. This is OK for me. We can solve this conversation.

bschonec · 2025-01-14T16:12:32Z

I'm having trouble figuring out where to put the "if create_dir = true" tests. Should the test be in defines/server_export_spec.rb or in classes/nfs_spec.rb?

smortex · 2025-01-14T18:52:54Z

manifests/functions/create_export.pp

+      if $create_dir {
+        file { $name:
+          ensure => directory,
+          owner  => $owner,
+          group  => $group,
+          mode   => $mode,
+        }


Given it will not manage the resource, I suggest to name the new param manage rather than create (i.e. the file resource create a non-existing directory but also ensure owner, group, permissions, etc).

If $create_dir is true (which is the default) then the directory is managed. Or am misunderstanding your comment? My use case here is that I have a 3rd party application that expects the directory to be absent. The application install will fail if the directory is created but I still have to export the directory via NFS. This solves the problem of exporting the directory but not being required to create the directory/export.
My pull request doesn't change the default behavior of creating the directory.

Sorry to be unclear, I am suggesting naming the parameter manage_dir rather than create_dir.

Or manage_directory as bytes are cheap nowadays 😄

Because if you set manage_directory => false and you put the wrong owner / group on the directory you created by hand, puppet will not fix this because it does not manage that directory.

Changed per your recommendations. Thanks.

manifests/functions/create_export.pp

wyardley · 2025-01-15T04:02:43Z

manifests/functions/create_export.pp

@@ -15,13 +15,18 @@
 # @param mode
 #   Sets the permissions of the exported directory.
 #
+# @param create_dir
+#   Boolean.  Create the directory to be exported.


nit, but the type and default get added when you build Reference.md using puppet-strings, so both should be omitted here. You might want to reword it to something along the lines of:

Suggested change

# Boolean. Create the directory to be exported.

# Whether or not to create the directory to be exported.

(and then remove line 20 entirely).

wyardley · 2025-01-15T04:02:56Z

manifests/server/export.pp

@@ -47,6 +47,10 @@
 # @param v4_export_name
 # @param nfsv4_bindmount_enable
 #
+# @param create_dir
+#   Boolean.  Create the directory to be exported.
+#   Defaults to true.


same feedback as the other parameter with generated docs.

Changed per your recommendations. Thanks.

bschonec mentioned this pull request May 20, 2024

Allow creating the export without having to create the directory. #184

Open

bschonec force-pushed the option_for_create_nfs_exported_directory branch from 5eb710c to 4916df4 Compare May 20, 2024 15:27

bschonec force-pushed the option_for_create_nfs_exported_directory branch from b2e91e8 to a164ebe Compare June 10, 2024 12:09

h-haaks reviewed Jun 10, 2024

View reviewed changes

bschonec force-pushed the option_for_create_nfs_exported_directory branch 3 times, most recently from 1a9fa3c to 79e32d1 Compare June 10, 2024 12:55

tuxmea reviewed Jun 11, 2024

View reviewed changes

bschonec force-pushed the option_for_create_nfs_exported_directory branch 3 times, most recently from 6e9b743 to bcf5d29 Compare June 13, 2024 11:17

bschonec closed this Jan 14, 2025

bschonec force-pushed the option_for_create_nfs_exported_directory branch from 9582efa to a77b94a Compare January 14, 2025 15:57

bschonec reopened this Jan 14, 2025

smortex reviewed Jan 14, 2025

View reviewed changes

bastelfreak reviewed Jan 14, 2025

View reviewed changes

manifests/functions/create_export.pp Show resolved Hide resolved

bschonec force-pushed the option_for_create_nfs_exported_directory branch from a16a483 to 1ca9169 Compare January 14, 2025 22:28

wyardley reviewed Jan 15, 2025

View reviewed changes

wyardley added the enhancement label Jan 15, 2025

bschonec force-pushed the option_for_create_nfs_exported_directory branch 4 times, most recently from c7b2fdb to 7499141 Compare January 16, 2025 11:16

Add the option to create the exported directory automatically

e057306

bschonec force-pushed the option_for_create_nfs_exported_directory branch from 7499141 to e057306 Compare January 16, 2025 11:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Option for create nfs exported directory #186

Option for create nfs exported directory #186

bschonec commented May 20, 2024

tuxmea commented Jun 10, 2024

bschonec commented Jun 10, 2024

h-haaks Jun 10, 2024

tuxmea Jun 13, 2024

h-haaks Jun 13, 2024

tuxmea Jun 11, 2024

bschonec Jun 11, 2024 •

edited

Loading

h-haaks Jun 11, 2024 •

edited

Loading

bschonec Jun 12, 2024

tuxmea Jun 12, 2024 •

edited

Loading

bschonec Jun 12, 2024

kenyon Jun 12, 2024

tuxmea Jun 13, 2024

bschonec commented Jan 14, 2025

smortex Jan 14, 2025

bschonec Jan 14, 2025 •

edited

Loading

smortex Jan 15, 2025

bschonec Jan 16, 2025

wyardley Jan 15, 2025

wyardley Jan 15, 2025

bschonec Jan 16, 2025

	# Boolean. Create the directory to be exported.
	# Whether or not to create the directory to be exported.

Option for create nfs exported directory #186

Are you sure you want to change the base?

Option for create nfs exported directory #186

Conversation

bschonec commented May 20, 2024

tuxmea commented Jun 10, 2024

bschonec commented Jun 10, 2024

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

bschonec Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

h-haaks Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuxmea Jun 12, 2024 • 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

bschonec commented Jan 14, 2025

Choose a reason for hiding this comment

bschonec Jan 14, 2025 • 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

bschonec Jun 11, 2024 •

edited

Loading

h-haaks Jun 11, 2024 •

edited

Loading

tuxmea Jun 12, 2024 •

edited

Loading

bschonec Jan 14, 2025 •

edited

Loading