search

MycroftAI / mycroft-core

Mycroft Core, the Mycroft Artificial Intelligence platform.
https://mycroft.ai
Apache License 2.0
6.48k stars 1.27k forks source link

Improve help message and logging of dev_setup.sh #3105

Closed mikejgray closed 2 years ago

mikejgray commented 2 years ago

Description

Corrects incidental findings from review of dev_setup.sh (fixes https://github.com/MycroftAI/mycroft-core/issues/1714). Most of the findings have been corrected already. This PR should close what little remains, aside from a security concern with log file permissions that probably belongs in a separate issue.

How to test

Execute dev_setup.sh in various operating systems Execute shellcheck dev_setup.sh for linting and best practices

Contributor license agreement signed?

CLA [x] (Whether you have signed a CLA - Contributor Licensing Agreement

devops-mycroft commented 2 years ago

Voight Kampff Integration Test Succeeded (Results)

forslund commented 2 years ago

Looks like a nice improvement. I think the github action is running into an issue when the log folder hasn't been created and given the correct permissions. Maybe move that stage to the start of the setup?

mikejgray commented 2 years ago

Checked locally, should be ok this time.

codecov-commenter commented 2 years ago

Codecov Report

Merging #3105 (0512c52) into dev (6af4313) will decrease coverage by 0.00%. The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev    #3105      +/-   ##
==========================================
- Coverage   54.05%   54.04%   -0.01%     
==========================================
  Files         120      120              
  Lines       11032    11032              
==========================================
- Hits         5963     5962       -1     
- Misses       5069     5070       +1
Impacted Files Coverage Δ
mycroft/tts/tts.py 77.81% <0.00%> (-0.30%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6af4313...0512c52. Read the comment docs.

mikejgray commented 2 years ago

Probably not a bad idea. We can also explicitly call out that the new logfile exists and may have further information. Should I include that in this PR?

forslund commented 2 years ago

I think it's a good idea @krisgesling do you have any opinion?

mikejgray commented 2 years ago

I went ahead and added it in.

krisgesling commented 2 years ago

Hey sorry, I was away last week but this looks great.

Agree on the final success message too :+1:

My only question is whether we need to use the -a, --append flag for all the tee calls? Currently it seems to overwrite the log file with each new line.

mikejgray commented 2 years ago

Thanks @krisgesling , good call-out. I'll be sure to have a better test setup next time. I've submitted that fix.

mikejgray commented 2 years ago

@forslund Anything else you'd like me to do on this PR?