Project

General

Profile

Actions
Copy link

Bug #4462

closed

Bundle permissions_type_recursion() with 'automatic detection' use a recursion with files

Added by Nicolas PERRON almost 11 years ago. Updated over 2 years ago.

Status:
Released
Priority:
1 (highest)
Assignee:
Nicolas PERRON
Category:
Generic methods
Target version:
ncf-0.x
Pull Request:
https://github.com/Normation/ncf/pull/39
Severity:
UX impact:
User visibility:
Effort required:
Priority:
0
Name check:
Fix check:
Regression:

Description

The bundle permissions_type_recusion() can detect automatically if target is a file or folder but the behavior is not good.
When the target is a directory, the recursion is explicitly set to 0 and if this is file, a recursion is applied (but it applies on directories too).

Actions
Copy link
 #1

Updated by Nicolas PERRON almost 11 years ago

  • Status changed from New to Pending technical review
  • % Done changed from 0 to 100
  • Pull Request set to https://github.com/Normation/ncf/pull/39

Pull Request URL added: https://github.com/Normation/ncf/pull/39

Jon, could you review it please ?

Actions
Copy link
 #2

Updated by Nicolas PERRON almost 11 years ago

  • Assignee changed from Nicolas PERRON to Jonathan CLARKE
Actions
Copy link
 #3

Updated by Jonathan CLARKE over 10 years ago

  • Status changed from Pending technical review to Discussion
  • Assignee changed from Jonathan CLARKE to Nicolas PERRON

Nicolas, I don't understand what the problem is here.

The current implementation applies two promises to a directory (one for it's contents, the same as if it was a file, and one for the directory itself).

What is wrong here? Please explain your problem, and include in your PR an acceptance test to demonstrate the problem, and prove that it is fixed, and to ensure it never comes back.

Actions
Copy link
 #4

Updated by Nicolas PERRON over 10 years ago

Jonathan CLARKE wrote:

Nicolas, I don't understand what the problem is here.

The current implementation applies two promises to a directory (one for it's contents, the same as if it was a file, and one for the directory itself).

What is wrong here? Please explain your problem, and include in your PR an acceptance test to demonstrate the problem, and prove that it is fixed, and to ensure it never comes back.

The problems are:
  • The use of body depth_search 'recurse' display messages: "warning: depth_search (recursion) is promised for a base object '[...]' that is not a drectory"
    • We should apply depth_search only on folders so we need to detect them to make a distinction between files and folders.
  • The use of body depth_search 'recurse' on directories will apply the permissions into the folder content and subfolder but not on the folder itself. This is due to the attribute 'include_basedir' which is set to false by default.
    • We should use the body depth_search 'recurse_with_base' instead which set 'include_basedir' to 'true'.
Actions
Copy link
 #5

Updated by Jonathan CLARKE over 10 years ago

Nicolas PERRON wrote:

Jonathan CLARKE wrote:

Nicolas, I don't understand what the problem is here.

The current implementation applies two promises to a directory (one for it's contents, the same as if it was a file, and one for the directory itself).

What is wrong here? Please explain your problem, and include in your PR an acceptance test to demonstrate the problem, and prove that it is fixed, and to ensure it never comes back.

The problems are:
  • The use of body depth_search 'recurse' display messages: "warning: depth_search (recursion) is promised for a base object '[...]' that is not a drectory"
    • We should apply depth_search only on folders so we need to detect them to make a distinction between files and folders.

Ah OK, agreed.

  • The use of body depth_search 'recurse' on directories will apply the permissions into the folder content and subfolder but not on the folder itself. This is due to the attribute 'include_basedir' which is set to false by default.
    • We should use the body depth_search 'recurse_with_base' instead which set 'include_basedir' to 'true'.

This is already addressed by an acceptance test (https://github.com/Normation/ncf/blob/master/tree/30_generic_methods/permissions_type_recursion.cf), and that is what the "is_target_directory" class is used for in this method. I don't understand why this doesn't work?

Actions
Copy link
 #6

Updated by Nicolas CHARLES over 10 years ago

Jonathan CLARKE wrote:

Nicolas PERRON wrote:

Jonathan CLARKE wrote:

Nicolas, I don't understand what the problem is here.

The current implementation applies two promises to a directory (one for it's contents, the same as if it was a file, and one for the directory itself).

What is wrong here? Please explain your problem, and include in your PR an acceptance test to demonstrate the problem, and prove that it is fixed, and to ensure it never comes back.

The problems are:
  • The use of body depth_search 'recurse' display messages: "warning: depth_search (recursion) is promised for a base object '[...]' that is not a drectory"
    • We should apply depth_search only on folders so we need to detect them to make a distinction between files and folders.

Ah OK, agreed.

  • The use of body depth_search 'recurse' on directories will apply the permissions into the folder content and subfolder but not on the folder itself. This is due to the attribute 'include_basedir' which is set to false by default.
    • We should use the body depth_search 'recurse_with_base' instead which set 'include_basedir' to 'true'.

This is already addressed by an acceptance test (https://github.com/Normation/ncf/blob/master/tree/30_generic_methods/permissions_type_recursion.cf), and that is what the "is_target_directory" class is used for in this method. I don't understand why this doesn't work?

It's a bit more tricky. If I'm correct, Nicolas tried to set permission 650 on the folder, but it failed
It's a feature (bug?) of CFEngine, you cannot set permission 4xx or 6xx on a folder, it will automatically change it to 5xx or 7xx (note that you can 2xx and 1xx)
I'm not sure it is a real issue (who wants to 6xx a folder ?)

Actions
Copy link
 #7

Updated by Nicolas PERRON over 10 years ago

  • Assignee changed from Nicolas PERRON to Jonathan CLARKE

Nicolas CHARLES wrote:

Jonathan CLARKE wrote:

Nicolas PERRON wrote:

Jonathan CLARKE wrote:

Nicolas, I don't understand what the problem is here.

The current implementation applies two promises to a directory (one for it's contents, the same as if it was a file, and one for the directory itself).

What is wrong here? Please explain your problem, and include in your PR an acceptance test to demonstrate the problem, and prove that it is fixed, and to ensure it never comes back.

The problems are:
  • The use of body depth_search 'recurse' display messages: "warning: depth_search (recursion) is promised for a base object '[...]' that is not a drectory"
    • We should apply depth_search only on folders so we need to detect them to make a distinction between files and folders.

Ah OK, agreed.

  • The use of body depth_search 'recurse' on directories will apply the permissions into the folder content and subfolder but not on the folder itself. This is due to the attribute 'include_basedir' which is set to false by default.
    • We should use the body depth_search 'recurse_with_base' instead which set 'include_basedir' to 'true'.

This is already addressed by an acceptance test (https://github.com/Normation/ncf/blob/master/tree/30_generic_methods/permissions_type_recursion.cf), and that is what the "is_target_directory" class is used for in this method. I don't understand why this doesn't work?

It's a bit more tricky. If I'm correct, Nicolas tried to set permission 650 on the folder, but it failed
It's a feature (bug?) of CFEngine, you cannot set permission 4xx or 6xx on a folder, it will automatically change it to 5xx or 7xx (note that you can 2xx and 1xx)
I'm not sure it is a real issue (who wants to 6xx a folder ?)

You're right, my test failed because I've tried to set permission 650 on a folder and as you said this is a feature/bug in CFEngine.

Nevertheless, I've made these changes in order to stop having these warning: warning: depth_search (recursion) is promised for a base object '[...]' that is not a drectory which add noises into the logs.

Actions
Copy link
 #8

Updated by Nicolas CHARLES over 10 years ago

  • Assignee changed from Jonathan CLARKE to Nicolas PERRON

Nicolas,

Thank you for this PR !
I've made a slight remark on the recurse_with_base if the class is_type_files is defined
Could you correct this and self merge ?

thank you !

Actions
Copy link
 #9

Updated by Nicolas PERRON over 10 years ago

  • Status changed from Discussion to Pending release

Applied in changeset commit:db6e5aefb1d643c014cbd44793ad460b5d07727b.

Actions
Copy link
 #10

Updated by Nicolas PERRON over 10 years ago

Applied in changeset commit:5fb1aabc6a6cc746ee3e8ba51e25c19f6ff6e792.

Actions
Copy link
 #11

Updated by Vincent MEMBRÉ about 8 years ago

  • Status changed from Pending release to Released
Actions
Copy link
 #12

Updated by Alexis Mousset over 2 years ago

  • Target version changed from 0.x to ncf-0.x
  • Priority set to 0
Actions
Copy link
 #13

Updated by Alexis Mousset over 2 years ago

  • Project changed from 41 to Rudder
  • Category set to Generic methods
Actions
Copy link

Also available in: Atom PDF