pybind/mgr: generalize CLICheckNonemptyFileInput() error msg #41378

varshar16 · 2021-05-18T09:45:14Z

This PR makes the following changes:

checks if file contains only spaces
generalize CLICheckNonemptyFileInput() error msg
use CLICheckNonemptyFileInput decorator in nfs module

Fixes: https://tracker.ceph.com/issues/50858

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox

src/pybind/mgr/dashboard/services/access_control.py

src/pybind/mgr/mgr_module.py

varshar16 · 2021-05-19T06:10:59Z

Fixed mypy errors

mgr_module.py: note: In function "CLICheckNonemptyFileInput":
mgr_module.py:428: error: Incompatible return value type (got "Callable[[Callable[..., Tuple[int, str, str]]], Callable[..., Tuple[int, str, str]]]", expected "Callable[..., Tuple[int, str, str]]")
dashboard/services/auth.py:16: note: In module imported here,
dashboard/module.py:33: note: ... from here,
dashboard/__init__.py:55: note: ... from here:
dashboard/services/access_control.py:552: error: Argument 1 to "__call__" of "CLICommand" has incompatible type "Tuple[int, str, str]"; expected "Callable[..., Tuple[int, str, str]]"
dashboard/services/access_control.py:691: error: Argument 1 to "__call__" of "CLICommand" has incompatible type "Tuple[int, str, str]"; expected "Callable[..., Tuple[int, str, str]]"
dashboard/services/access_control.py:845: error: Argument 1 to "__call__" of "CLICommand" has incompatible type "Tuple[int, str, str]"; expected "Callable[..., Tuple[int, str, str]]"
dashboard/services/access_control.py:867: error: Argument 1 to "__call__" of "CLICommand" has incompatible type "Tuple[int, str, str]"; expected "Callable[..., Tuple[int, str, str]]"
dashboard/module.py:36: note: In module imported here,
dashboard/__init__.py:55: note: ... from here:
dashboard/settings.py: note: In function "handle_option_command":
dashboard/settings.py:243: error: "Tuple[object, ...]" not callable
dashboard/controllers/iscsi.py:20: note: In module imported here,
dashboard/plugins/feature_toggles.py:11: note: ... from here,
dashboard/module.py:54: note: ... from here,
dashboard/__init__.py:55: note: ... from here:
dashboard/services/iscsi_cli.py:25: error: Argument 1 to "__call__" of "CLICommand" has incompatible type "Tuple[int, str, str]"; expected "Callable[..., Tuple[int, str, str]]"

varshar16 · 2021-05-19T08:14:04Z

Fixed flake8 error

lint run-test: commands[0] | flake8
[NON-XML-CHAR-0x1B][0;36m./services/access_control.py[NON-XML-CHAR-0x1B][0m:[NON-XML-CHAR-0x1B][1;33m31[NON-XML-CHAR-0x1B][0m:[NON-XML-CHAR-0x1B][1;32m1[NON-XML-CHAR-0x1B][0m: [NON-XML-CHAR-0x1B][1;31mE302[NON-XML-CHAR-0x1B][0m expected 2 blank lines, found 1
1     E302 expected 2 blank lines, found 1

varshar16 · 2021-05-19T14:31:29Z

nfs tests passed: https://pulpito.ceph.com/varsha-2021-05-19_13:30:21-rados-wip-check-file-inputs-nfs-distro-basic-smithi/

batrick · 2021-05-20T14:41:14Z

@ceph/dashboard please run this through your QA when you can.

epuertat

LGTM @varshar16 ! Just left a few suggestions over there.

epuertat · 2021-05-24T14:47:43Z

src/pybind/mgr/mgr_module.py

+ERROR_MSG_EMPTY_INPUT_FILE = 'File is empty!!'
+ERROR_MSG_NO_INPUT_FILE = 'No Input File!!'


Nit, but 2 exclamation marks might sound a bit... over-emphatic :P

I just grepped our source code (grep "[\"'][^\"']*\![^\"'][\"']" * -Ri --include="*.[py, h*,c*]") and found that it's not frequent in Ceph code (only in some deps, like zstd or civetweb).

What about using the POSIX errno style:

Empty input file: {details}

Input file not specified: {details}

This is probably more of a discussion for docs experts: @knortema ?

Interesting stack overflow discussion. I have updated the message.

epuertat · 2021-05-24T14:49:49Z

src/pybind/mgr/mgr_module.py

+                return -errno.EINVAL, '', f'{ERROR_MSG_NO_INPUT_FILE} Please specify the file '\
+                                          f'containing {content} with "-i" option'


What about using HandleCommandResult() helper instead of the raw tuple (retval, stdout, stderr)?

Suggested change

return -errno.EINVAL, '', f'{ERROR_MSG_NO_INPUT_FILE} Please specify the file '\

f'containing {content} with "-i" option'

return HandleCommandResult(

-errno.EINVAL,

'',

f'{ERROR_MSG_NO_INPUT_FILE} Please specify the file '\

f'containing {content} with "-i" option'

)

Reverted this change. As it requires multiple files to be updated. It is best to be done in a different PR.

src/pybind/mgr/mgr_module.py

varshar16 · 2021-05-25T11:29:36Z

@epuertat Thanks for the review. I have updated the PR, please take another look.

epuertat

Thanks for addressing my comments. Looks good, @varshar16!! (worthy to add some exclamation marks :-P)

varshar16 · 2021-05-25T13:33:21Z

jenkins test dashboard

github-actions · 2021-05-25T20:43:12Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

Signed-off-by: Varsha Rao <varao@redhat.com>

Fixes: https://tracker.ceph.com/issues/50858 Signed-off-by: Varsha Rao <varao@redhat.com>

Signed-off-by: Varsha Rao <varao@redhat.com>

varshar16 · 2021-05-26T06:11:35Z

Rebased

varshar16 · 2021-05-26T10:39:34Z

jenkins test make check

varshar16 · 2021-05-26T10:40:02Z

NFS Tests Passed: https://pulpito.ceph.com/varsha-2021-05-26_09:43:29-rados-wip-check-file-inputs-nfs-distro-basic-smithi/

tchaikov · 2021-05-27T07:22:35Z

failures are not related.

varshar16 requested a review from a team as a code owner May 18, 2021 09:45

varshar16 requested review from pereman2 and Waadkh7 and removed request for a team May 18, 2021 09:45

github-actions bot added dashboard mgr pybind labels May 18, 2021

varshar16 requested review from alfonsomthd, batrick and a team May 18, 2021 09:45

varshar16 added nfs needs-review labels May 18, 2021

alfonsomthd reviewed May 18, 2021

View reviewed changes

src/pybind/mgr/dashboard/services/access_control.py Outdated Show resolved Hide resolved

src/pybind/mgr/mgr_module.py Outdated Show resolved Hide resolved

varshar16 force-pushed the wip-check-file-inputs-nfs branch 2 times, most recently from a9d2f2a to cbd344d Compare May 19, 2021 06:09

varshar16 requested a review from alfonsomthd May 19, 2021 06:11

alfonsomthd approved these changes May 19, 2021

View reviewed changes

alfonsomthd requested a review from epuertat May 19, 2021 06:29

varshar16 added the wip-varsha-testing label May 19, 2021

varshar16 removed the wip-varsha-testing label May 19, 2021

Waadkh7 approved these changes May 20, 2021

View reviewed changes

batrick approved these changes May 20, 2021

View reviewed changes

batrick added needs-qa and removed needs-review labels May 20, 2021

epuertat approved these changes May 24, 2021

View reviewed changes

varshar16 force-pushed the wip-check-file-inputs-nfs branch from a872cce to c805146 Compare May 25, 2021 11:23

varshar16 force-pushed the wip-check-file-inputs-nfs branch from c805146 to c132a96 Compare May 25, 2021 11:52

epuertat approved these changes May 25, 2021

View reviewed changes

github-actions bot added the needs-rebase label May 25, 2021

varshar16 added 4 commits May 26, 2021 11:38

pybind/mgr: check if file contains only spaces

2d32f5f

Signed-off-by: Varsha Rao <varao@redhat.com>

pybind/mgr: generalize CLICheckNonemptyFileInput() error msg

2dfd009

Signed-off-by: Varsha Rao <varao@redhat.com>

mgr/nfs: use CLICheckNonemptyFileInput decorator

6193f38

Fixes: https://tracker.ceph.com/issues/50858 Signed-off-by: Varsha Rao <varao@redhat.com>

mgr/dashboard/access_control: fix flake8 expected 2 blank lines error

a026ef2

Signed-off-by: Varsha Rao <varao@redhat.com>

varshar16 force-pushed the wip-check-file-inputs-nfs branch from c132a96 to a026ef2 Compare May 26, 2021 06:11

github-actions bot removed the needs-rebase label May 26, 2021

tchaikov added the wip-kefu-testing label May 26, 2021

tchaikov merged commit 8bc2c6d into ceph:master May 27, 2021

varshar16 deleted the wip-check-file-inputs-nfs branch May 27, 2021 09:16

varshar16 mentioned this pull request Jun 29, 2021

pacific: nfs backport June #42096

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pybind/mgr: generalize CLICheckNonemptyFileInput() error msg #41378

pybind/mgr: generalize CLICheckNonemptyFileInput() error msg #41378

varshar16 commented May 18, 2021

varshar16 commented May 19, 2021

varshar16 commented May 19, 2021

varshar16 commented May 19, 2021

batrick commented May 20, 2021

epuertat left a comment

epuertat May 24, 2021

varshar16 May 25, 2021

epuertat May 24, 2021

varshar16 May 25, 2021

varshar16 May 25, 2021 •

edited

varshar16 commented May 25, 2021

epuertat left a comment

varshar16 commented May 25, 2021

github-actions bot commented May 25, 2021

varshar16 commented May 26, 2021

varshar16 commented May 26, 2021

varshar16 commented May 26, 2021

tchaikov commented May 27, 2021

		ERROR_MSG_EMPTY_INPUT_FILE = 'File is empty!!'
		ERROR_MSG_NO_INPUT_FILE = 'No Input File!!'

		return -errno.EINVAL, '', f'{ERROR_MSG_NO_INPUT_FILE} Please specify the file '\
		f'containing {content} with "-i" option'

pybind/mgr: generalize CLICheckNonemptyFileInput() error msg #41378

pybind/mgr: generalize CLICheckNonemptyFileInput() error msg #41378

Conversation

varshar16 commented May 18, 2021

varshar16 commented May 19, 2021

varshar16 commented May 19, 2021

varshar16 commented May 19, 2021

batrick commented May 20, 2021

epuertat left a comment

Choose a reason for hiding this comment

epuertat May 24, 2021

Choose a reason for hiding this comment

varshar16 May 25, 2021

Choose a reason for hiding this comment

epuertat May 24, 2021

Choose a reason for hiding this comment

varshar16 May 25, 2021

Choose a reason for hiding this comment

varshar16 May 25, 2021 • edited

Choose a reason for hiding this comment

varshar16 commented May 25, 2021

epuertat left a comment

Choose a reason for hiding this comment

varshar16 commented May 25, 2021

github-actions bot commented May 25, 2021

varshar16 commented May 26, 2021

varshar16 commented May 26, 2021

varshar16 commented May 26, 2021

tchaikov commented May 27, 2021

varshar16 May 25, 2021 •

edited