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

Add build status to project upload #542

Merged
merged 7 commits into from
Aug 23, 2021
Merged

Conversation

anthmatic
Copy link
Contributor

@anthmatic anthmatic commented Aug 20, 2021

Description and Context

After successful project upload, polls for build status

Screenshots

Spinners while project dependencies are building
image

Success message
image

General failure
image

Failure of a single dependency build
image

Copy link
Contributor

@miketalley miketalley left a comment

Choose a reason for hiding this comment

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

Looks good, just one question about adding the spinnies dependency.

@@ -4,10 +4,15 @@ const path = require('path');
const chalk = require('chalk');
const findup = require('findup-sync');
const { prompt } = require('inquirer');
const Spinnies = require('spinnies');
Copy link
Contributor

@miketalley miketalley Aug 23, 2021

Choose a reason for hiding this comment

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

We were using ora for loading spinners elsewhere in the CLI. Is there a reason to have both or should we use ora here also?

It looks like having multiple spinners displayed simultaneously for the purpose of displaying the status of the subbuilds is not possible using ora. I think we should work on updating uses of ora to use spinnies in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's exactly it and I agree

@miketalley miketalley requested a review from drewjenkins August 23, 2021 21:11
build.status === BUILD_STATUS.SUCCESS ||
build.status === BUILD_STATUS.FAILURE
);
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

😀

Copy link
Contributor

@miketalley miketalley left a comment

Choose a reason for hiding this comment

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

Looks good after testing and digging in a bit more.

@miketalley miketalley merged commit 696ec8b into master Aug 23, 2021
@miketalley miketalley deleted the projects/build-status branch August 23, 2021 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants