knife client create should check file permissions #11241

snehaldwivedi · 2021-03-25T13:45:33Z

Description

Support user-friendly error checking on file permissions for knife client create
knife user create is already covered in PR

Related Issue

#3514
#3517

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

I have read the CONTRIBUTING document.
I have run the pre-merge tests locally and they pass.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.
All commits have been signed-off for the Developer Certificate of Origin.

lamont-granquist · 2021-03-29T17:14:57Z

lib/chef/knife/client_create.rb

+        if config[:file]
+          file = config[:file]
+          unless File.exist?(file) ? File.writable?(file) : File.writable?(File.dirname(file))
+            ui.fatal "File #{config[:file]} is not writable.  Check permissions."


This should really be two different conditionals with two different error messages. When the file does not exist and the directory is not writable the error message should be separate and there should really be two different conditionals here for readability.

tas50 · 2021-04-01T21:20:52Z

@snehaldwivedi This will need to get rebased for the recent refactoring we did to knife.

marcparadise · 2021-06-03T16:35:17Z

knife/lib/chef/knife/client_create.rb

+            exit 1
+          end
+
+          unless File.writable?(file)


If the file does not exist, the File.writable? will return false:

yes, if the file is writable then only it will return true else false. You can also go through documentation here. Now as per your example:

File.writable?("does-not-exist") it has checked if the given file is writable or not and returns false as it doesn't exist.

File.write("does-not-exist", "blah") it returns 4. Here it will create a new file "does-not-exist" and add a content "blah" can returns length of the content.

File.writable?("does-not-exist") now after 2nd point where the file is not created now File.writable? is return true.

My concern is less functional, and more user experience - it is correct that the directory is not writable and the action should fail.

But in the case where the directory for /tmp/test/afile.out does not exist, we will report an error that Dir '/tmp/test/' is not writable. Check permissions. This is misleading, and we could provide more actionable feedback than "check permissions". It would be better to specifically verify that the directory exists and report that error independently of permissions-related errors.

@marcparadise Got your point. I Will update the condition accordingly.

marcparadise · 2021-07-08T14:08:59Z

knife/lib/chef/knife/client_create.rb

+            exit 1
+          end
+        else
+          ui.fatal "#{type} #{file} dose not exist."


Should be "does not exist."

marcparadise

This good and will be ready to merge once the typo (dose -> does) is fixed.

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

snehaldwivedi requested review from a team as code owners March 25, 2021 13:45

snehaldwivedi mentioned this pull request Mar 25, 2021

knife client create should check file permissions #10380

Closed

10 tasks

snehaldwivedi force-pushed the snehal/Knife_client_create_should_check_file_permissions branch 3 times, most recently from 2db12b6 to 3b66e32 Compare March 26, 2021 04:50

lamont-granquist suggested changes Mar 29, 2021

View reviewed changes

snehaldwivedi force-pushed the snehal/Knife_client_create_should_check_file_permissions branch from aad0550 to e103941 Compare March 31, 2021 13:49

snehaldwivedi force-pushed the snehal/Knife_client_create_should_check_file_permissions branch from e103941 to 944aee7 Compare April 5, 2021 06:01

snehaldwivedi force-pushed the snehal/Knife_client_create_should_check_file_permissions branch 2 times, most recently from 1a55a98 to 7e46d2a Compare May 7, 2021 12:29

marcparadise requested changes Jun 3, 2021

View reviewed changes

snehaldwivedi force-pushed the snehal/Knife_client_create_should_check_file_permissions branch 3 times, most recently from f466976 to 7edc105 Compare June 24, 2021 08:08

snehaldwivedi self-assigned this Jul 6, 2021

marcparadise reviewed Jul 8, 2021

View reviewed changes

marcparadise requested changes Jul 8, 2021

View reviewed changes

snehaldwivedi force-pushed the snehal/Knife_client_create_should_check_file_permissions branch 2 times, most recently from 7cac3b5 to 2bf5fb8 Compare July 9, 2021 08:03

snehaldwivedi added 8 commits July 15, 2021 22:00

Knife client create should check for file permissions

8303eb4

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

Fixed test failing on windows for create client

0d3b877

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

Fix chefstyle

828b62e

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

Fix chefstyle

f2336b4

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

Updated writable condition for check file permissions

ab4f5bf

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

Fixed failing test for client create

b25ddb3

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

Improved fatal condition to check file & dir exists

7aefe3d

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

to fix failing testcases

faabcc8

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

snehaldwivedi added 3 commits July 15, 2021 22:00

Refactord code

587d710

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

Fixed spellcheck

476fc83

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

To fixed typo

4a71edf

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

snehaldwivedi force-pushed the snehal/Knife_client_create_should_check_file_permissions branch from 2bf5fb8 to 6ea8158 Compare July 16, 2021 05:01

Fix chefstyle

fb087a1

Signed-off-by: snehaldwivedi <sdwivedi@msystechnologies.com>

snehaldwivedi force-pushed the snehal/Knife_client_create_should_check_file_permissions branch from 6ea8158 to fb087a1 Compare July 16, 2021 05:03

tas50 changed the title ~~Snehal/knife client create should check file permissions~~ knife client create should check file permissions Jul 28, 2021

tas50 merged commit 96e5907 into master Jul 28, 2021

tas50 deleted the snehal/Knife_client_create_should_check_file_permissions branch July 28, 2021 18:41

jasonwbarnett mentioned this pull request Aug 31, 2021

Bug: knife client create #11985

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

knife client create should check file permissions #11241

knife client create should check file permissions #11241

snehaldwivedi commented Mar 25, 2021

lamont-granquist Mar 29, 2021

tas50 commented Apr 1, 2021

marcparadise Jun 3, 2021

snehaldwivedi Jun 4, 2021

marcparadise Jun 21, 2021 •

edited

snehaldwivedi Jun 22, 2021

marcparadise Jul 8, 2021

marcparadise left a comment

knife client create should check file permissions #11241

knife client create should check file permissions #11241

Conversation

snehaldwivedi commented Mar 25, 2021

Description

Related Issue

Types of changes

Checklist:

lamont-granquist Mar 29, 2021

Choose a reason for hiding this comment

tas50 commented Apr 1, 2021

marcparadise Jun 3, 2021

Choose a reason for hiding this comment

snehaldwivedi Jun 4, 2021

Choose a reason for hiding this comment

marcparadise Jun 21, 2021 • edited

Choose a reason for hiding this comment

snehaldwivedi Jun 22, 2021

Choose a reason for hiding this comment

marcparadise Jul 8, 2021

Choose a reason for hiding this comment

marcparadise left a comment

Choose a reason for hiding this comment

marcparadise Jun 21, 2021 •

edited