Closed RollingHog closed 1 year ago
And I can't see exactly which error in pretty big Travis feed fails. Never used tests in Python, sry.
Hi @RollingHog, thanks for your changes. I would see it implemented a bit differently:
list_configs
, I think we should just return existing short_config type with a new field "parsing_failed: true". Returning 2 completely different types from a method is more difficult to read and maintainRegarding the tests, you can run them using the follwing command from src
folder:
python3 -m unittest discover -s tests -p "*.py" -t .
Oookay.
Will update this during work
Instead of returning completely different type from list_configs, I think we should just return existing short_config type with a new field "parsing_failed: true".
if a script file contains text: "allowed_users", then it shouldn't be listed at all (otherwise users might see scripts, which are not intended for them)
In all other places in config_service, if we are loading extended model for a config, we should check this flag as well. I.e. if a user tries to load and display such a script, we would show proper error
On UI, instead of using a separate group, I would suggest to show the script as it is, but: make the text red and add error icon to it
Admin UI should be adjusted as well, i.e. the config should be listed, but shouldn't be clickable, and shown as read
Kinda. Not sure what is "shown as read"
Questions emerged:
allowed_users
: should I regexp JSON server-side and return something like "possibly restricted script" instead of its real filename for non-admin users? Exceptions
: should I hide their tracebacks from log since this is considered as acceptable option now?On allowed_users: should I regexp JSON server-side and return something like "possibly restricted script" instead of its real filename for non-admin users?
Yes please, please check the file (I think regex is not needed, just check, that the file content contains "allowed_users" string) However, do not show anything to non-admin users at all. The file might be supposed to be hidden from them
For the stacktrace, I would suggest to log it. It will cause some noise, but it's easier to investigate having traceback
For the stacktrace, I would suggest to log it. It will cause some noise, but it's easier to investigate having traceback
I meant only for "cannot parse" error of course. Still log?
And about files. I think a note "some file with restricted access is malformed" will be good, wont it? Or there is some reason to fully hide this info?
I meant only for "cannot parse" error of course. Still log?
yes, it can still provide some hints on what's wrong with the file.
I think a note "some file with restricted access is malformed" will be good, wont it? Or there is some reason to fully hide this info?
Not for normal users. Any restricted information should be hidden from users, to avoid any leaks. Even if it's only a file name
Even if it's only a file name
No no no! What I planned to do was replacing filename with "file with possibly restricted access" server-side. So users see that something is wrong (and can eg notify admin) but don't know what exactly.
yes, it can still provide some hints on what's wrong with the file.
You mean that error message about exact line where problem is? Okay.
What I planned to do was replacing filename with "file with possibly restricted access" server-side.
Ah, I see. I thought, that you wanted to display filename and show the error May be in this case, it would be just enough to show how many files failed to load, e.g. in the script list UI it would look like this:
All done. Now trying to understand testing command output to fix the tests.
@RollingHog I'm so sorry, I totally forgot about this PR. I thought that we finished it. I'm merging it to master now, thanks for your work!
Well, I hope it helped.
521 adding notice for failed parsing
TESTS NOT FIXED, WAITING FOR OVERALL APPROVAL/CHANGES
UI changes:
conf/runners
.json files script-server cannot parse, red group called `` will appear in main scripts listBackend changes:
list_configs()
now returns mixedList[ Union[ShortConfig, GetShortConfigFailedError] ]
list which is filtered inGetScripts()
server.py
GetScripts()
endpoint - now returns JSON{'scripts': scripts, 'failed': failed}
where'failed'
is "failed parsing"Frontend changes (I had no idea what I'm doing):
ScriptFailedGroup.vue
closely resemblingScriptListGroup.vue
to show list of non-interactive unparsed files