18F / marigold

An experimental set of reusable components and patterns for making websites and applications at 18F/TTS.
http://federalist.18f.gov.s3-website-us-east-1.amazonaws.com/site/18F/marigold/
Other
10 stars 6 forks source link

Build on Federalist; remove build dir from git #24

Closed shawnbot closed 7 years ago

shawnbot commented 7 years ago

This PR solves #23 by removing the build directory from git and leveraging Federalist's new npm script support. This means no more convoluted Jekyll -> plugin -> shell script -> npm messiness! I've also provided a fix for #29 along the way.

You can see it on Federalist staging! It also generates a build.log to provide us with some clues if things go wrong in the future.

Two things need to happen on Federalist before we should see this working in production:

shawnbot commented 7 years ago

I'm pushing another commit to test out the Federalist build.

shawnbot commented 7 years ago

Still no dice, and the Federalist log says, "This build completed successfully." 😢

shawnbot commented 7 years ago

🚨 IT WORKS!! 🚨

msecret commented 7 years ago

The build process makes sense, I say :+1:

shawnbot commented 7 years ago

I'm refactoring to bring back the build directory and simplify the Jekyll setup; fingers crossed this builds too.

shawnbot commented 7 years ago

If this build is successful, fractal.js should no longer exist because it's never copied to the build directory.

shawnbot commented 7 years ago

Okay, so that previous build failed. I'm fixing up the Jekyll config to exclude the src directory and outputting the build log in build/log.txt so we can view it directly in the browser more easily.

juliaelman commented 7 years ago

@shawnbot nice work! ^5

At first glance, I think it is b/c this command is expecting the build dir to already be available we are having some breakage on Federalist.

shawnbot commented 7 years ago

Hmm, I thought that I did that in 39f6763440a2d374d75f0e7a3571649f6a8cc1e9, but it looks like the build/.gitkeep file didn't take.

shawnbot commented 7 years ago

Okay, I'm going to roll this back to dbeadf9752d94f5e22de183df08b154ce0f0f680 so we can close it this and move on.

shawnbot commented 7 years ago

We are back in business. @juliaelman, let's :shipit:

juliaelman commented 7 years ago

Making a note here from our conversation in Slack. The error is now solved, but the preview on the component page is now not showing, screenshot below:

screen shot 2016-11-01 at 3 45 46 pm

Let's leave this PR until we have a conversation with @NoahKunin and @wslack tomorrow. It seems that we're stretching the limits of Federalist and it might be time to switch to cloud.gov 👍

shawnbot commented 7 years ago

Update! We're good to use Federalist, and we're going to work with the team to add more explicit support for npm-driven builds. In the meantime, we should figure out what we need to do to fix the broken previews in this branch.

shawnbot commented 7 years ago

Okay, so I opened up the Federalist previews so they're publicly visible, but now there's a new problem: I think that Federalist is setting the X-Frame-Options: DENY header on the S3 bucket, which Chrome dutifully obeys and prevents component previews in <iframe> elements from loading on pages like this one:

image

Strangely, this doesn't happen on the master preview. cc @jmhooper

shawnbot commented 7 years ago

Update: @jmhooper tells me that the X-Frame-Options header is set for all federalist.18f.gov responses. This effectively renders the <iframe> elements that Fractal relies upon to sandbox component previews unusable in Federalist branch previews. I've escalated this to a request for having preview URLs excluded from the header, but until then we're hosed. 😢

wslack commented 7 years ago

cc @NoahKunin to check this out per our brief office convo

More explanation from @shawnbot here: https://github.com/18F/federalist/issues/706

NoahKunin commented 7 years ago

Un-subbing since I approved this elsewhere. Nice work!

shawnbot commented 7 years ago

Okay, good times! Here's where we are right now:

wslack commented 7 years ago

@shawnbot one note, that https://github.com/18F/federalist/issues/180 has been opened in place of https://github.com/18F/federalist/issues/708

wslack commented 7 years ago

@shawnbot any negative impacts of increasing the memory limit? Can that be another issue for discussion on the Federalist repo?

shawnbot commented 7 years ago

Thanks, @wslack. Everyone: I've updated the PR description with the latest state of things and added a checklist for the remaining tasks, as well as a note that I've fixed #29 too.

shawnbot commented 7 years ago

@wslack I'm not sure about the memory limits; I think it just means that our container resources are more "expensive" in whatever way we pay for them. cc @jmhooper

juliaelman commented 7 years ago

@shawnbot just wanted to touch base on this issue, as there a lot of moving parts around the Githubs. It looks like there are three steps left in order for this pull request to be merged in?

shawnbot commented 7 years ago

@juliaelman It's true: this is super complicated. Two things to be updated on Federalist in production in order for this to move forward: the new build containers with npm support, and upped memory limits for them. I don't have a sense yet of if/when this will happen, but @wslack and @jmhooper are aware that these are blockers for this project.

shawnbot commented 7 years ago

I think the necessary changes for this (npm and 1G memory on build containers) are available on the "new" (GovCloud) Federalist. @wslack or @jmhooper, can you confirm?

shawnbot commented 7 years ago

OBE 💥