-
-
Notifications
You must be signed in to change notification settings - Fork 40
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
Some refactoring #3
Conversation
To run, do ./build && bin/transcode from the root of the repo. The basedir to look up scripts is currently **not** configurable.
Can now do simple go build && ./go-transcode
I would omit those points:
Reverse proxy should be encouraged, I agree, but regarding that is it basically almost no overhead in implementing, it could stay as an option. Even cconnecting to a reverse proxy might benefit from HTTPs (for various usecases). Proxy flag is for the future, if My main inspiration in code skeleton comes from other project that I maintain https://github.com/m1k1o/neko and https://github.com/m1k1o/neko-rooms where I try to match patterns. Regarding project layout, I am trying to stick to the offical standard: https://github.com/golang-standards/project-layout |
I think it's bad security to have TLS in static build which does not receive system security upgrades. i can add nginx reverse proxy to docker by default if good with you... or add warning log that TLS mode should never be trusted?
that link is not a standard it's more like a starter kit (explained in readme), but was made before go modules existed. i don't know too much for golang so if you're happy with structure i'll just add some "architecture" lines to readme to understand easier |
That is a fair point. We can add warning message when starting a server.
Regarding the architecture, I would like to keep is similar to the other projects I am maintaining. It is then easier for me to change between projects. But I am also not against new folder structure, if this project could benefit from that. |
Done
that's fair, do you think:
so instead of bin/data/dev we have bin or dev . less visual information when browsing repository what do you think? is it compatible with your architecture? |
most boxes are checked
|
Dev is used only as helpers during development. Bin is meant for build artifacts (not checked to repostitory).
I think at this point,
Those two test files simulate with transcoding for testing purposes. Normally, you would call profile with file source. These test files have just dummy input.
We can use go:embed (I used it here) to add it to binary.
Yes, I would like to try it with docker and add my changes. I already have some refactoring ideas in my mind. If you don't mind, I'll push it to this branch, github allows it for PRs (if user does not explicitly disable it). |
Sure, let's try not force push and make sure we don't push broken things |
before basedir config it was necessary to add it here
tests/reload.sh works with spaces in file
OK i
i think only blocking issue is panic from review; rest can be written TODO or opened another issue:
agree? |
Yes, I agree, that we should merge changes and then address new changes in different PRs. Although one thing, that changed with this PR is name of the default config file. Before, it was |
ok are we good? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thank you for your contribution!
remove Server.Proxy unused(moved to reverse proxying #4)remove TLS support (Server.Key and Server.Cert) encourage reverse proxyadd warning that TLS mode should not be used for securitymove CMD parsing in internal/config to cmd packagefuturemove everything docker to packaging/docker dir?futuretranscodeStart takes URL not stream namefutureinfo about code flow for request processfuturefeedback welcome (i'm new to golang)
also there's a lot of stuff in root dir it's littlee confusing, do you like to move source to src/ (not sure golang likes that), build/test scripts (dev/ data/ and tests) to bin/, and profiles mixed together, like this: