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

Integrated bindata for public and templates #74

Closed
wants to merge 12 commits into from
Closed

Conversation

tboerger
Copy link
Member

@tboerger tboerger commented Nov 4, 2016

Fixes #30

Things left to do:

  • Integrate the switch for mailer templates
  • Make conf also aware of builtin or external

@lunny
Copy link
Member

lunny commented Nov 5, 2016

LGTM

@strk
Copy link
Member

strk commented Nov 5, 2016

Does public/ already support override via custom/public?
As I think I'm using public/img to store custom logo and favicon...

@thibaultmeyer
Copy link
Contributor

thibaultmeyer commented Nov 5, 2016

I think bindata.go dont have to be committed on repository and must be auto-generated during the make process.

It become a mess when two (or more) Pull Requests update different files (eg : app.ini for the first commit and add new files for the second) because the bindata.go file cause merge error each time.

@strk
Copy link
Member

strk commented Nov 5, 2016

I tend to agree with @0xBAADF00D but would rather avoid blocking this change due to that issue. Rather, I suggest a new issue is filed for the bindata.go threatment.

@tboerger
Copy link
Member Author

tboerger commented Nov 5, 2016

I'm currently thinking about rebuilding the bindata interface that handles static directories instead of embedding the data, switched via a build tag.

@codecov-io
Copy link

codecov-io commented Nov 5, 2016

Current coverage is 2.18% (diff: 0.00%)

Merging #74 into master will not change coverage

@@            master       #74   diff @@
========================================
  Files           31        31          
  Lines         7508      7508          
  Methods          0         0          
  Messages         0         0          
  Branches         0         0          
========================================
  Hits           164       164          
  Misses        7327      7327          
  Partials        17        17          

Powered by Codecov. Last update 91b589f...b58e8fc

@bkcsoft bkcsoft added the pr/wip This PR is not ready for review label Nov 14, 2016
@andreynering
Copy link
Contributor

andreynering commented Nov 16, 2016

@tboerger I don't know if it helps you, but I like fileb0x because it prevents the need of two different tools: bindata and bindata-assetfs.

Also, bindata-assetfs has this bug on Windows.

@bkcsoft
Copy link
Member

bkcsoft commented Nov 28, 2016

@andreynering +1 on not using go-bindata-assets WRT elazarl/go-bindata-assetfs#34 (comment)

One of the reasons I'm not putting too much effort in this project, is
because there are better alternative available.

@tboerger tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Nov 28, 2016
@tboerger
Copy link
Member Author

fileb0x won't work because we rely on tags with the new bindata.

@bkcsoft
Copy link
Member

bkcsoft commented Nov 29, 2016

@tboerger
Copy link
Member Author

I will avaluate it with go.rice, looks like it also supports loading from a directory natively.

@andreynering
Copy link
Contributor

@tboerger

I will evaluate it with go.rice, looks like it also supports loading from a directory natively.

Like this? UnnoTed/fileb0x#3

Just trying to help, I'm fine with any option 👍

@tboerger
Copy link
Member Author

@andreynering currently we want to embed the data optionally via a build tag, bindata builds all assets into the binary while !bindata always reads the assets from the filesystem.

@andreynering
Copy link
Contributor

You're right, this would not be possible using fileb0x and build tags.

A workaroud would be having two fileb0x files ( fileb0x-debug.yml and fileb0x.yml) and two distinct commands on makefile to build with one or other option.

But if rice supports build tags, it would be way simpler.

@tboerger tboerger added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Nov 29, 2016
@tboerger
Copy link
Member Author

tboerger commented Dec 5, 2016

Obsolete via #354 #314 #293

@tboerger tboerger closed this Dec 5, 2016
@tboerger tboerger added issue/duplicate The issue has already been reported. and removed type/enhancement An improvement of existing functionality lgtm/need 1 This PR needs approval from one additional maintainer to be merged. status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR pr/wip This PR is not ready for review labels Dec 5, 2016
@tboerger tboerger removed this from the 1.0.0 milestone Dec 5, 2016
@tboerger tboerger deleted the bindata branch December 5, 2016 19:42
lunny pushed a commit to lunny/gitea that referenced this pull request Feb 7, 2019
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/duplicate The issue has already been reported.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Embed bindata optionally
7 participants