Skip to content

Don't commit assets #199

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.

Sign up for GitHub

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

Jump to bottom
Merged
smacker merged 6 commits into from
Mar 7, 2019
Merged

Don't commit assets #199

smacker merged 6 commits into from
Mar 7, 2019

Conversation

@smacker
Copy link
Collaborator

@smacker smacker commented Mar 4, 2019

fix #192

Now it works the same as any other apps project.
I also replaced bindata with esc.
static_files_dev.go & static_files.go are 100% copy-paste from lookout-web.

Makefile still looks a bit strange but changing it is out of the scope.

@smacker smacker requested review from carlosms and dpordomingo March 4, 2019 16:42
@carlosms
Copy link
Collaborator

carlosms commented Mar 5, 2019

It's tested this locally, and for me make build & make serve fail if the dir ./server/asset does not exist.
When the dir exists, make serve works, but make packages creates a bin serving the 501 errors.

@smacker
Copy link
Collaborator Author

smacker commented Mar 5, 2019

thanks for testing! I'll re-test with a clean state.

@carlosms
Copy link
Collaborator

carlosms commented Mar 5, 2019

Also if it's not too complex it would be good to test it in travis. Maybe a make packages, run the bin in the background, and then check the wget localhost:port exit code.

@smacker
Copy link
Collaborator Author

smacker commented Mar 5, 2019

good idea! 👍

smacker added 6 commits March 6, 2019 12:49
@smacker
delete commited assets
c49dc52 
Signed-off-by: Maxim Sukharev <max@smacker.ru>
@smacker
exclude generated assets from git
cc2ee6b 
Signed-off-by: Maxim Sukharev <max@smacker.ru>
@smacker
update Makefile to use esc instead of bindata
320d20b 
Signed-off-by: Maxim Sukharev <max@smacker.ru>
@smacker
use static files solution similar to other projects
c73c483 
Signed-off-by: Maxim Sukharev <max@smacker.ru>
@smacker
make sure directory for assets exists
6ed192d 
Signed-off-by: Maxim Sukharev <max@smacker.ru>
@smacker
add integration test
70d1848 
Signed-off-by: Maxim Sukharev <max@smacker.ru>
@smacker
Copy link
Collaborator Author

smacker commented Mar 6, 2019

  1. I added mkdir -p to make sure directory always exists
  2. I can't reproduce the problem with make packages. This command and make build on clean repository generate correct binary with assets included.
  3. I added simple integration test with check for correct content in / path
  4. Had to rebase because asset.go was changed in master

@smacker smacker mentioned this pull request Mar 6, 2019
Merged
dpordomingo
dpordomingo approved these changes Mar 7, 2019
View reviewed changes
Copy link
Member

@dpordomingo dpordomingo left a comment

Choose a reason for hiding this comment

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

don't like the idea of having code with subtle differences doing the same in different projects, but ok
LGTM

Makefile
GO_BUILD_ARGS := -ldflags "$(LD_FLAGS)" -tags "$(WITH_STATIC_TAG)"

# Environment and arguments to use in `go run` calls.
GO_RUN_ARGS += -tags "$(WITH_STATIC_TAG)"
Copy link
Member

@dpordomingo dpordomingo Mar 7, 2019

Choose a reason for hiding this comment

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

this didn't came from lookout-web

Copy link
Collaborator Author

@smacker smacker Mar 7, 2019

Choose a reason for hiding this comment

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

in lookout-web it looks like this: https://github.com/src-d/lookout/blob/master/Makefile.web#L69
same-same

Copy link
Member

@dpordomingo dpordomingo Mar 7, 2019

Choose a reason for hiding this comment

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

kind of same functionality, but subtle different code (ie. GO_RUN_ARGS does not appear in that Makefile.web)
But as I said, not blocker at all, so we can merge as it is.
(I also locally tested, and worked like a charm)

Copy link
Collaborator Author

@smacker smacker Mar 7, 2019

Choose a reason for hiding this comment

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

yeah. Maybe we can actually extract go-handler part & Makefile somewhere and then just re-use in our projects. @carlosms wdyt? Shall I create an issue for that?

Copy link
Member

@dpordomingo dpordomingo Mar 7, 2019

Choose a reason for hiding this comment

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

+1
I've been proposing that since years 🎉

Copy link
Collaborator

@carlosms carlosms Mar 7, 2019

Choose a reason for hiding this comment

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

sounds good to me. Let's open that issue :)

Copy link
Collaborator Author

@smacker smacker Mar 7, 2019

Choose a reason for hiding this comment

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

https://github.com/src-d/feature-idea/issues/129

Makefile
GO_RUN_ARGS += -tags "$(WITH_STATIC_TAG)"

GOCMD = go
GORUN = $(GOCMD) run $(GO_RUN_ARGS)
Copy link
Member

@dpordomingo dpordomingo Mar 7, 2019

Choose a reason for hiding this comment

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

not in lookout-web

@dpordomingo
Copy link
Member

dpordomingo commented Mar 7, 2019

pretty good idea adding integration tests ! 🎉

carlosms
carlosms reviewed Mar 7, 2019
View reviewed changes
server/cmd/bblfsh-web/main.go
// we handle urls on frontend
r.NoRoute(func(c *gin.Context) { c.File(indexPath) })

// TODO(@smacker): add configuration for ServerURL and FooterHTML
Copy link
Collaborator

@carlosms carlosms Mar 7, 2019

Choose a reason for hiding this comment

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

This is functionality brought from the code c&p from lookout, but we can't use yet in bblfsh web, right?
Should we have an issue for this, to implement it later?

Copy link
Collaborator Author

@smacker smacker Mar 7, 2019

Choose a reason for hiding this comment

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

#201

carlosms
carlosms approved these changes Mar 7, 2019
View reviewed changes
Copy link
Collaborator

@carlosms carlosms left a comment

Choose a reason for hiding this comment

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

👍

@smacker smacker merged commit be2fc33 into bblfsh:master Mar 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dpordomingo dpordomingo dpordomingo approved these changes

@carlosms carlosms carlosms approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Use the same way to handle go-bindata as other projects

3 participants

@smacker @carlosms @dpordomingo