Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve help message and logging of dev_setup.sh #3105

Merged
merged 8 commits into from
Jun 24, 2022
Merged

Improve help message and logging of dev_setup.sh #3105

merged 8 commits into from
Jun 24, 2022

Conversation

mikejgray
Copy link
Contributor

Description

Corrects incidental findings from review of dev_setup.sh (fixes #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 devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label May 21, 2022
@devops-mycroft
Copy link

devops-mycroft commented May 21, 2022

Voight Kampff Integration Test Succeeded (Results)

@forslund
Copy link
Collaborator

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
Copy link
Contributor Author

Checked locally, should be ok this time.

@codecov-commenter
Copy link

codecov-commenter commented May 21, 2022

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%) ⬇️

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.

forslund
forslund previously approved these changes May 22, 2022
Copy link
Collaborator

@forslund forslund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these changes looks good now and the CI is happy. Do you think it would be a good idea to add a "setup complete" line at the end to explicitly show that the dev-setup completed successfully?

@mikejgray
Copy link
Contributor Author

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
Copy link
Collaborator

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

@mikejgray
Copy link
Contributor Author

I went ahead and added it in.

@krisgesling
Copy link
Contributor

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

Agree on the final success message too 👍

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
Copy link
Contributor Author

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

@mikejgray mikejgray requested a review from forslund June 7, 2022 13:25
@mikejgray
Copy link
Contributor Author

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

Copy link
Contributor

@krisgesling krisgesling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Mike this is all working great now!

@krisgesling krisgesling merged commit 1b67bf3 into MycroftAI:dev Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incidental findings from review of dev_setup.sh
5 participants