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

Static assets now built into the binary #63

Merged
merged 1 commit into from
Sep 25, 2018

Conversation

sporkmonger
Copy link
Contributor

Problem

Fixes #7, simplifies deployment.

Solution

Bake static assets into a .go file that functions as a file server.

Notes

Opted to use https://github.com/rakyll/statik over https://github.com/jteeuwen/go-bindata since it's actively maintained.

fsHandler, err := loadFSHandler()
if err != nil {
logger.Fatal(err)
os.Exit(1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I meant to check whether .Fatal calls os.Exit or not before committing. Looks like it does, so this line isn't needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That said this is one of those "this should never happen" errors.

@sporkmonger
Copy link
Contributor Author

I opted to check internal/auth/statik/statik.go into source control so that you wouldn't need to call go generate immediately after a clone to get the project to compile and because I suspect it won't change often. But I could also easily see the argument in the other direction since it's a generated file.

@sporkmonger
Copy link
Contributor Author

sporkmonger commented Sep 19, 2018

So it seems like the gpm install in the earlier build step maybe isn't doing what I'm expecting it to?

Edit
Narrator: It wasn't actually gpm install.

@sporkmonger
Copy link
Contributor Author

OK, closer, but maybe an issue with wrong version of Go?

@sporkmonger
Copy link
Contributor Author

Failures that only happen in CI/CD are always fun.

@sporkmonger
Copy link
Contributor Author

I'm going to squash the fighting-with-CircleCI commits.

@sporkmonger
Copy link
Contributor Author

Ready for review.

@shrayolacrayon
Copy link

Thanks for opening @sporkmonger! We'll be working on reviewing and getting this merged as soon as we can.

@shrayolacrayon
Copy link

One more request - we should be able to also get rid of copying over the static directory in the Dockerfile when building the docker image.

Since we are still working on figuring out a good way to build and push docker images for forked community contributions, I've pushed a copy of your pr branch on buzzfeed/sso (https://github.com/buzzfeed/sso/tree/pr/63) to validate on our internal infrastructure so that we can get this merged!

@sporkmonger
Copy link
Contributor Author

sporkmonger commented Sep 22, 2018 via email

@sporkmonger sporkmonger force-pushed the static-assets branch 2 times, most recently from 139c3a6 to 9055aca Compare September 24, 2018 20:11
@shrayolacrayon
Copy link

Thanks for making those changes @sporkmonger, this looks good to me, can you please squash the commits and I'll approve and merge!

* Uses https://github.com/rakyll/statik to embed files
* Minor changes to build, now uses Go 1.10
* Dockerfile no longer copies static files
@sporkmonger
Copy link
Contributor Author

All squashed!

@shrayolacrayon shrayolacrayon merged commit a946076 into buzzfeed:master Sep 25, 2018
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