Skip to content

Conversation

abeisgoat
Copy link
Contributor

Hi, welcome to PR, I'll bring around a round of waters to get you started.

Today's special is a series of artisan fixes from free-range developers.

First, we update the firepit-builder Dockerfile to use node@12, which has the decadent side-effect of shifting firepit builds to using node@12 as well. This is surely to delight your palette while keeping you hungry for more.

We're also thrilled to offer a flight of changes to firepit-builder/pipeline.js which compliment each other and come together into what we're calling Maintenant, le script de pipeline fonctionne sur Windows or (in English) "Now the pipeline script works on Windows." The largest issue was the config templating system used find/replace for paths, which did not handle Window's \ paths, as some of our patrons noticed that \ is an escape character in JSON, which produced a very conflicting flavor for some.

We're also thrilled to say, in the back kitchen, we've introduced the fame chef shx who will be helping us ensure that our chmod calls are cross-platform. Of course, he won't influence our primetime package.json menu and will stay as a core part of the firepit-builder section.

Aside from helping our Yelp reviews, we also believe that these updates should mend the issues raised in #1796 and hopefully stabilize the Windows builds by no longer corrupting the config files generated from our templates.

Thanks again for joining us tonight, I'll check back in a moment to get your orders.

@googlebot googlebot added the cla: yes Manual indication that this has passed CLA. label Jan 7, 2020
@abeisgoat abeisgoat requested a review from samtstern January 7, 2020 21:18
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 65.603% when pulling 2b24c9e on ah-pipeline-win into 9ca80cf on master.

@abeisgoat abeisgoat requested a review from bkendall January 7, 2020 21:50
@@ -1,4 +1,4 @@
FROM node:10
FROM node:12
Copy link
Contributor

Choose a reason for hiding this comment

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

Not opposed but was there any particular reason you made this update? Was something not working on 10?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's some issues with pkg with node@10 and by bumping to 12 we just sidestep that ordeal.

@samtstern
Copy link
Contributor

Changes LGTM. And seems like you're having fun over there.

@abeisgoat abeisgoat merged commit b33625a into master Jan 8, 2020
@abeisgoat abeisgoat deleted the ah-pipeline-win branch January 8, 2020 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Manual indication that this has passed CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants