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

Some refactoring #3

Merged
merged 56 commits into from
Sep 26, 2021
Merged

Some refactoring #3

merged 56 commits into from
Sep 26, 2021

Conversation

klahaha
Copy link
Contributor

@klahaha klahaha commented Sep 24, 2021

  • streams are loaded from viper config, not streams.yaml
  • cmd is built by go build in repo root (main package)
  • support /etc/transcode
    • remove hardcoded paths
    • config file is loaded from /etc/transcode or ./
    • profiles/data is served from /etc/transcode or ./
    • everything is relative to basedir, which is passed via CLI, or is /etc/transcode (if exists), or is ./
  • config file is automatically reloaded in serve mode
  • remove some unused stuff
    • removed Config type
    • merge some small files together for clarity
    • remove Server.Proxy unused (moved to reverse proxying #4)
    • remove TLS support (Server.Key and Server.Cert) encourage reverse proxy add warning that TLS mode should not be used for security
  • move CMD parsing in internal/config to cmd package future
  • fix Docker build
  • Write some tests
    • settings cascade (test_args.sh)
    • settings auto-reload
  • Use settings for ffmpeg profiles folder
    • Move all to profiles/default and profiles/nvidia
    • Use a profiles directory setting to find the profiles
  • make room in repo root
    • embed data/play.html in binary from internal/api/hls/
    • move data/*-test.sh to tests/
    • remove data dir from repo
    • remove bin dir from repo
    • move everything docker to packaging/docker dir? future
  • transcodeStart takes URL not stream name future
  • Update readme
    • build and run with docker
    • build and run without docker
    • info about project structure
    • info about code flow for request process future

feedback 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:

  • bin/
  • src/
  • profiles/
  • what about docker files? is there a standard directory to place it?
  • LICENSE README.md go.mod go.sum ...

@m1k1o
Copy link
Owner

m1k1o commented Sep 24, 2021

I would omit those points:

  • remove Server.Proxy unused
  • remove TLS support (Server.Key and Server.Cert) encourage reverse proxy

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 X-Forwarded-For headers should be trusted or not.

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

@klahaha klahaha mentioned this pull request Sep 24, 2021
@klahaha
Copy link
Contributor Author

klahaha commented Sep 24, 2021

Proxy flag is for the future, if X-Forwarded-For headers should be trusted or not.

#4

remove TLS support (Server.Key and Server.Cert) encourage reverse proxy

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?

Regarding project layout, I am trying to stick to the offical standard

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

@m1k1o
Copy link
Owner

m1k1o commented Sep 24, 2021

or add warning log that TLS mode should never be trusted?

That is a fair point. We can add warning message when starting a server.

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

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.

@klahaha
Copy link
Contributor Author

klahaha commented Sep 24, 2021

We can add warning message when starting a server.

Done

I would like to keep is similar to the other projects I am maintaining

that's fair, do you think:

  • remove bin or dev (use one folder not two)
  • move data/test* to tests/ (what are these files for?)
  • move data/play.html to internal/api/ where it's served.. maybe directly in source code (it's just a demo users will not use it) or use compile time macro to load it?

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?

@klahaha
Copy link
Contributor Author

klahaha commented Sep 24, 2021

most boxes are checked

  • do you want to do docker stuff? i can do it but i don't have nvenc so i cant test nvidia profiles
  • move cobra/cmd parsing to cmd package.. not sure to do it with limited golang skill but i will try
  • update README: i added some stuff, more ideas?

@m1k1o
Copy link
Owner

m1k1o commented Sep 24, 2021

remove bin or dev (use one folder not two)

Dev is used only as helpers during development. Bin is meant for build artifacts (not checked to repostitory).

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?

I think at this point, /bin is unused, since our only build artifact is one executable binary named go-transcode and not checked in repository.

move data/test* to tests/ (what are these files for?)

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.

move data/play.html to internal/api/ where it's served.. maybe directly in source code (it's just a demo users will not use it) or use compile time macro to load it?

We can use go:embed (I used it here) to add it to binary.

do you want to do docker stuff? i can do it but i don't have nvenc so i cant test nvidia profiles

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).

@klahaha
Copy link
Contributor Author

klahaha commented Sep 24, 2021

If you don't mind, I'll push it to this branch

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
@klahaha klahaha mentioned this pull request Sep 26, 2021
@klahaha
Copy link
Contributor Author

klahaha commented Sep 26, 2021

OK i

  • pushed single profile directory, i think it's good but if you don't like ill revert
  • moved data/*-test.sh to profiles/ not tests/ because profiles/ will be in basedir (/etc/transcode maybe). tests is only in repository. hls-test.sh is not used ?
  • finish config reload (i think)

i think only blocking issue is panic from review; rest can be written TODO or opened another issue:

  • better README
  • transcodeStart takes URL
  • move CMD parsing in internal/config to cmd package
  • move everything docker to packaging/docker dir?
  • transcodeStart takes URL not stream name

agree?

@m1k1o
Copy link
Owner

m1k1o commented Sep 26, 2021

i think only blocking issue is panic from review; rest can be written TODO or opened another issue.

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 streams.yaml and now it is transcode.yaml. Maybe, we could rename it to config.yaml, because context of the software, where it is running already says directory (e.g. /etc/transcode/transcode.yaml vs /etc/transcode/config.yaml).

@klahaha
Copy link
Contributor Author

klahaha commented Sep 26, 2021

ok are we good?

@klahaha klahaha changed the title [WIP] Some refactoring Some refactoring Sep 26, 2021
Copy link
Owner

@m1k1o m1k1o left a 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!

@m1k1o m1k1o merged commit c318f3b into m1k1o:master Sep 26, 2021
This was referenced Sep 26, 2021
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.

None yet

2 participants