General observations about POC version

[umputun]

First of all, thank you for taking care of this project; I appreciate it.

After a quick review, I have prepared a list of potential improvements:

• Generally, the project seems to be a little bit overengineered. For example, I see no reason to introduce swag tooling just to produce a service with a couple of endpoints.
• Health check is way too much for what is needed. Practically all we want is a /health endpoint returning 200 or non-200 and a simple text (or JSON) message for a failed response.
• The project structure doesn't match other Go parts of the radio-t infrastructure. For instance, we never called Config what actually seems to be a CLI param; we use the go-flags package for this.
• It looks like it has way too many packages, and some (e.g., Client) have confusing names. It took me time to figure out which Client we even need here and for what exactly. I think making a single package like recorder or something similar would be perfectly fine.
• Different linter settings lead to different styles. I would prefer to adopt our typical settings for the linter. For instance, having comments on public methods is a good idea, and the linter should enforce it.
• I don't know if all those web dependencies are even needed, e.g., Bootstrap and web components. In the end, all we need is a simple list of all recordings grouped under episode number directories and a way to download each one of them or a whole directory as a tar.gz. To me, a simple CSS or a tiny CSS-styles framework like picocss or even something smaller will be enough.
• Running shell-related things like df -h + h.dir + | awk 'NR==2 {gsub("%","",$5); print $5}' inside the app is not a good idea. I'm not sure if Go has a proper way to get disk utilization, but if not, we can just drop it.
• Tests are lacking. I'm not even sure what existing tests are doing, but one I'd like to see for sure is a full end-to-end test, bringing a test container with Icecast and streaming something for real.
• At the unit level, the code doesn't seem to be very testable either as it doesn't use any dependency injection. By the way, we are using Moq everywhere to generate mocks.
• Files needed for local development/testing, like .air.toml and .pre-commit-config.yaml, should not be in the repository.

By and large, I'd like to see the project resembling other parts, e.g., super-bot, tg-spam, publisher, rss_generator. The reason for this is that a similar structure will make long-term maintenance easier for someone familiar with other parts of this infrastructure. For the same reason, I would like to simplify the structure and keep only what we really need without adding unnecessary abstractions and specialized tooling. I think the structure can be as simple as main.go and main_test.go (maybe with test data too) at the app level and a server package for everything else. Well, we can also have a recorder package if we really need it. Probably all we should have in the server package is a server.go handling all the routes and a recorder doing the recording, plus test files and mock files.

A couple of functional requirements I have not mentioned before:

• We don't want to record 24/7 as the "next episode". Probably we may have some timeframe (i.e. 2h prior to streaming start and 4h after). Everything else we may record during a week, most likely some specials, and they probably can go to special-yyyymmdd or something like this.
• It will be nice to keep up to N days of recording, probably 30 days by default make sense. The rest of the files should be purged automatically.

[umputun]

cc @ar2rworld

[umputun]

Running shell-related things

It appears that syscall.Statfs func can replace the df call.

[umputun]

after #5 merge:

main.go

        wg.Add(1)
        go func() {
            defer wg.Done()
            s := server.NewServer(opts.Port, opts.Dir, revision)
            go s.Start()
        }()

• I don't get it, goroutine started from another goroutine? Probably a mistake.

• no need to execute run() in a goroutine to make it blocking the next line. Just run it directly.

• Server's Start() is non-cancelable from the outside. Making ctx in main and triggering it with NotifyContext will allow you to cancel this context, and you can pass it to the server.

    if revision == "local" {
        slog.SetLogLoggerLevel(slog.LevelDebug)
    }

• Looks way too opinionated to me. As a user, I want to be able to run with the debug info as needed by passing --dbg.

• Internal var names in main are non-idomatic. Probably myclient can be just c or if you preffer menengful var names - rtAPIClient. O even better, you may no need a var for this, just recorder.NewListener(recorder.NewRecorder(opts.Dir)). streamlistener probably just listener or streamListener if you preffer it this way.

• Side note: you seem to have a lot of //nolint. If you have to suppress some linters that often, you probably don't want them. Take a look at the .golangci.yml we use in other parts. I'd suggest adopting it to keep the same style of what we consider valid (for linter) and what not.

• Options formattion with spaces just wrong, i have removed it in my commit.

[umputun]

package recorder:

• just put two structs from models.go to recorder.go, no need to make more files
• client.go not tested
• FetchStream not using passed contexts but making a new one
• return entries[0].Title can panic because len(entries) > 0 is not checked prior to this
• I'd suggest to pass HttpClient interface with Do method to NewClient, this way you could mock it and test everything locally
• Clientlike is an odd name for the interface, maybe ClientService instead?
• all the check for os.IsNotExist(err) before MkdirAll is not needed, it won't fail if directory exists
• Record ignores context and essentially non-cancalable
• Listener, Stream and Recorder relationship is not very clear. Probably adding comment(s) explaining how one returns another and why it's doing it will help. To me it looks way too many moving parts. I see all of this as a single responsibility of Recorder

I also don't get how the main-level ticker is supposed to work with the recorder. To me, it looks like it will create a new stream every 5 seconds, and you will end up with multiple recorders storing the stream into the common file. If this is the case, this is not right. If not, probably some comments are needed around the ticker, explaining the flow. If this is the error as I think it is, it would be a very clear reason why this app needs real integration tests. Such types of high-level logical issues are very well captured by such tests.

[umputun]

server:

• no need to split Server methods across multiple files
• all the parts for index struct doesn't warrant a separate struct with its own methods just to read a list of files, just do it directly in the IndexHandler. If you see it's getting to big - extract to a helper function
• I'm not sure why DownloadRecordHandler even needed, practically we won't be able to even know what particular chunk we need and will download the whole zip
• none of structs/files in server package have any tests