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

check prerequisites 1 / 2 #2046

Closed
wants to merge 5 commits into from
Closed

check prerequisites 1 / 2 #2046

wants to merge 5 commits into from

Conversation

lc-thomasberger
Copy link
Member

@lc-thomasberger lc-thomasberger commented Aug 14, 2018

fixes #880

Checks known prerequisites in an early stage and exits if not met:

  • grunt-cli installed
  • git installed
  • github-api not accessible

Warns if not met:

  • [ ] local mongodb not found
  • ffmpeg installed (if enabled in config)

Part 2 will be to check if a connection to database can be established once it is configured. This will be based on #2015

- git
- grunt-cli 
- github-api connection
warn if local mongo-db not found
@lc-thomasberger lc-thomasberger self-assigned this Aug 14, 2018
@lc-thomasberger lc-thomasberger added the S: awaiting-review Completed issues waiting on reviews label Aug 14, 2018
@lc-thomasberger lc-thomasberger added this to the 0.5.1 milestone Aug 14, 2018
Copy link
Contributor

@tomgreenfield tomgreenfield left a comment

Choose a reason for hiding this comment

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

Grunt CLI and GitHub checks tested and working.

method: 'GET'
}, function(error, response) {
if (error) {
result.errors.push(`Failed to´connect to ${url}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Random character ´ in error string.

install.js Outdated
if(error) {
return handleError(error, 1, 'Failed to get the latest framework version. Check package.json.');
return handleError(error, 1, 'Not all required pre-requisites fulfilled. Please check install guides.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest removing required. A prerequisite is required by nature.

@@ -578,3 +579,71 @@ function error(msg) {
function isSilent() {
return process.env.SILENT;
}

/**
* Manually collect all misisng prerequisites
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: misisng.

], result, function(error) {
if (error) return callback(error);
if (result.errors.length) {
return callback(new Error(result.errors.join(', ')));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest a line break between errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

have tried this, will format it oddly. I can do however if you like.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the output for me in Windows if changed to .join('\n'):

Copy link
Member

Choose a reason for hiding this comment

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

Looks good.

Agree that it would look nicer with each error on a separate line.

Missing the following pre-requisites, please check install guides:
- git
- grunt
- ffmpeg
- failed to connect to github.com

lc-thomasberger and others added 3 commits August 14, 2018 17:33
split into check for required optional dependencies
add check for ffmpeg
change error output format
Copy link
Contributor

@tomgreenfield tomgreenfield left a comment

Choose a reason for hiding this comment

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

Formatted much better, thanks.

});
}

function checkLocalMongoDbConnection(result, callback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't actually check the connection does it? Maybe needs a rename to avoid any confusion.

checkLocalMongoDbConnection,
checkFfmpegInstalled
],
'Missing the following optional dependecies, please check install guides:',
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: dependecies.

@taylortom taylortom changed the base branch from develop to release/0.5.1 August 28, 2018 12:17
@taylortom taylortom changed the base branch from release/0.5.1 to develop August 31, 2018 09:58
@taylortom
Copy link
Member

Replaced with PR 2059.

@taylortom taylortom closed this Aug 31, 2018
@taylortom taylortom deleted the issue/880 branch September 4, 2018 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: awaiting-review Completed issues waiting on reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants