Bug #4462
closed
Bundle permissions_type_recursion() with 'automatic detection' use a recursion with files
Added by Nicolas PERRON about 10 years ago. Updated almost 2 years ago.
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).
Updated by Nicolas PERRON about 10 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 ?
Updated by Nicolas PERRON about 10 years ago
- Assignee changed from Nicolas PERRON to Jonathan CLARKE
Updated by Jonathan CLARKE about 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.
Updated by Nicolas PERRON about 10 years ago
Jonathan CLARKE wrote:
The problems are: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 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'.
Updated by Jonathan CLARKE about 10 years ago
Nicolas PERRON wrote:
Jonathan CLARKE wrote:
The problems are: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 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?
Updated by Nicolas CHARLES about 10 years ago
Jonathan CLARKE wrote:
Nicolas PERRON wrote:
Jonathan CLARKE wrote:
The problems are: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 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 ?)
Updated by Nicolas PERRON almost 10 years ago
- Assignee changed from Nicolas PERRON to Jonathan CLARKE
Nicolas CHARLES wrote:
Jonathan CLARKE wrote:
Nicolas PERRON wrote:
Jonathan CLARKE wrote:
The problems are: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 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.
Updated by Nicolas CHARLES almost 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 !
Updated by Nicolas PERRON almost 10 years ago
- Status changed from Discussion to Pending release
Applied in changeset commit:db6e5aefb1d643c014cbd44793ad460b5d07727b.
Updated by Nicolas PERRON almost 10 years ago
Applied in changeset commit:5fb1aabc6a6cc746ee3e8ba51e25c19f6ff6e792.
Updated by Vincent MEMBRÉ over 7 years ago
- Status changed from Pending release to Released
Updated by Alexis Mousset almost 2 years ago
- Target version changed from 0.x to ncf-0.x
- Priority set to 0
Updated by Alexis Mousset almost 2 years ago
- Project changed from 41 to Rudder
- Category set to Generic methods