Improve tasks CLI #821
Merged
Improve tasks CLI #821
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- to make it consistent with `npm run watch`
- no need to hide mapbox access tokens (there are public) - remove `export MAPBOX_ACCESS_TOKEN` step in contributing guide
- group sub-tasks in subroutines - DRY up using common module - add image test container setup sub-task such that `npm run pretest` on CircleCI runs the docker container and sets it up once and for all
- format bundle does not make sense in a non-watchify context
- use in watchified bundle module
- use in image test container tasks to display to the last build/plotly.js bundle time.
| - sudo ./tasks/run_in_testbed.sh mytestbed "export CIRCLECI=1 && node test/image/compare_pixels_test.js" | ||
| - sudo ./tasks/run_in_testbed.sh mytestbed "node test/image/export_test.js" | ||
| - npm run test-image | ||
| - npm run test-export |
There was a problem hiding this comment.
the local vs CI differences are now handled at the task-runner script level.
| updateVersion(constants.pathToPlotlyGeoAssetsSrc); | ||
| // copy topojson files from sane-topojson to dist/ | ||
| function copyTopojsonFiles() { | ||
| fs.copy(constants.pathToTopojsonSrc, constants.pathToTopojsonDist, |
There was a problem hiding this comment.
🐄 separate lines for each arg?
|
Great PR! After those little cows and things, 💃 ! |
| @@ -15,6 +15,7 @@ containerCommands.ping = [ | |||
| containerCommands.setup = [ | |||
| containerCommands.cpIndex, | |||
| containerCommands.restart, | |||
| 'sleep 1', | |||
There was a problem hiding this comment.
Looks like the npm run pretest is a little more flaky than the previous list of shell commands on CIrcleCi.
Not sure what's up.
I've added sleep delays:
- between the
nwrestart and the ping as well as as - between the docker run and setup commands.
Hopefully that will fix it. But we might have to look into this further.
This was referenced Aug 12, 2016
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolves #794
In brief
this PR:
tasks/in node which (1) will allow to Windows users to run them hassle-free (2) allow us to DRY up significant amounts of tasks code.pretestlifecycle task. It now contains everything needed to prep up the CircleCI box before testing including booting up the imagetest docker container (which cleans up ourcircle.ymlsignificantly)tasks/util/constants.js. No moreexport <mapbox-access-token>needed to start dev'ing!tasks/anddevtools/code.Ideas for future development
docker-composefor booting the imagetest container locally. That would mean one less dependency for devs 🎉 . Usenpm run pretestinstead (and maybe add annpm run postesttask to shut down the container while at it).