Skip to content

Add styleguide build command.#93

Closed
becw wants to merge 9 commits into
release-1.0from
styleguide-build-commands
Closed

Add styleguide build command.#93
becw wants to merge 9 commits into
release-1.0from
styleguide-build-commands

Conversation

@becw

@becw becw commented Aug 23, 2018

Copy link
Copy Markdown
Member

Completes ZRAD-119 - Add styleguide target to palantirnet/the-build.

To test:

  1. Require this branch of the-build in a project: composer require --dev palantirnet/the-build:dev-styleguide-build-commands
  2. Reinstall the-build: vendor/bin/the-build-installer
  3. When prompted, make sure that the styleguide directory and styleguide command are correct for your styleguide (if you miss the prompts, just re-run the installer)
  4. Run the build command: phing build -- this should install the styleguide dependencies and run the styleguide command that you configured
  5. Run the styleguide command directly: phing styleguide -- since your dependencies are already installed, this should just rerun the styleguide command

Open questions:

  • Will this cover most of our styleguide use cases? Folks working on the styleguide will continue to use the yarn/npm gulp-based tooling for development. The intent of this command is to generate the assets required for an artifact build.
  • This only supports composer and yarn right now -- and will only run install commands if those lockfiles are present. Do we need to also support npm?
    • Are we back to using npm only in some styleguides?
  • Where should we document that this can be configured on existing projects by:
    1. Adding the new <import> line for the styleguide targets to the project's build.xml
    2. Adding the phing call to styleguide to the existing build target, if desired
    3. Configuring the styleguide variables with phing styleguide-configure -Dbuild.env=default

Comment thread build.dist.xml
@byrond

byrond commented Sep 14, 2018

Copy link
Copy Markdown
Contributor

Running this by default in the VM will cause issues for devs using Yarn on their host machine. When there's a mismatch, it results in the following type of error:

Node Sass could not find a binding for your current environment: OS X 64-bit with Node.js 6.x

Found bindings for the following environments:
  - Linux 64-bit with Node.js 6.x

Would it be best to limit this to build.circle.properties by default?

@byrond

byrond commented Sep 14, 2018

Copy link
Copy Markdown
Contributor

For documentation, it might be a good idea to add a section to the project readme, with maybe a link to a new document in docs/ with details on updating existing projects to use the new styleguide commands. Also, it would probably be good to add the new properties to docs/properties.md.

Comment thread tasks/styleguide.xml

<!-- Target: styleguide -->
<target name="styleguide" description="Install and build the style guide.">

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this test whether the properties are set first? That way they can be only run on Circle, if desired.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's a good point -- especially re: the nodejs installs that vary by platform.

The thing is, where the styleguide is built tends to vary between development roles -- folks doing design and front end development will build it locally, but folks doing engineering, devops, or support will build the styleguide within the VM.

So I'm not sure where to go with this...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@becw and @russom-woldezghi, I think I have resolved this by checking for a linux-x64-59 binding in node-sass. If it isn't there, Phing will run yarn install, which actually allows you to use Butler on both the host and VM to build the styleguide (assuming the Node versions match).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tested by building the styleguide on my host, then running phing build in the VM, which ran successfully. I was also able to go back to my host and build the styleguide again.

@russom-woldezghi

Copy link
Copy Markdown

For documentation, it might be a good idea to add a section to the project readme, with maybe a link to a new document in docs/ with details on updating existing projects to use the new styleguide commands. Also, it would probably be good to add the new properties to docs/properties.md.

We could also point to have users run: vendor/bin/phing configure -Dbuild.env=your_environment to get all the possible options?

@russom-woldezghi russom-woldezghi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good overall. It would be nice to have a way to test against a Pantheon/Acquia environment to get close to production test run.

@byrond

byrond commented Oct 30, 2018

Copy link
Copy Markdown
Contributor

Requires palantirnet/butler#101 and palantirnet/the-vagrant#56 for new projects.

becw added a commit that referenced this pull request Nov 20, 2018
becw added a commit that referenced this pull request Nov 20, 2018
@becw becw added this to the 2.0 milestone Dec 3, 2018
@becw becw removed this from the 2.0 milestone Dec 14, 2018
@becw

becw commented Jan 9, 2019

Copy link
Copy Markdown
Member Author

Since we're not using Butler for new projects, I'm not going to worry about palantirnet/butler#101 :)

@becw

becw commented Jan 9, 2019

Copy link
Copy Markdown
Member Author

... and, I'm going to close this in favor of #117, which I just merged! Thank you, @byrond and @russom-woldezghi, for uncovering and pursuing these issues! Your work in this PR made it super easy to update this work for the release-2.0 branch.

@becw becw closed this Jan 9, 2019
@becw becw deleted the styleguide-build-commands branch January 9, 2019 01:05
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.

3 participants