[chore] refactor test/cliparsing.sh into a go test below internal/config

[LittleFox94]

It's now a normal Go test and does not need a precompiled GtS binary nor go run as it did before.

But the most important change is it now being independent from the declaration order of Configuration fields, as we aren't comparing a JSON dump anymore but test for specific fields instead.

[NyaaaWhatsUpDoc]

Looks mostly good so far :) just a few points:

• is there a reason that you went for putting the table driven tests in a separate YAML vs just having a slice of test structs in the test file itself?
• I think the test itself could do with some code commenting to help break it up and improve readability
• at the moment the test is heavily reliant on the binary already existing and the test always being called from the same directory, which is fine for running in the pipeline but a lot of people (myself included) tend to run them more locally

[LittleFox94]

• is there a reason that you went for putting the table driven tests in a separate YAML vs just having a slice of test structs in the test file itself?

that's actually an artifact of first refactoring it into a go script, will look at changing that :)

• I think the test itself could do with some code commenting to help break it up and improve readability

you mean the go code? Yeah probably

• at the moment the test is heavily reliant on the binary already existing and the test always being called from the same directory, which is fine for running in the pipeline but a lot of people (myself included) tend to run them more locally

yep, not too nice indeed .. best would probably not to execute a new binary but kinda load the config into the test process, if I can cleanly reset and reparse new arguments .. will look into that

[LittleFox94]

Oh another note: I'm actually doing this to have nicer test output for my refactoring of config generator stuff - generate way more things. Am quite far with that already, but the test output from the current cliparsing.sh was really slowing things down - hence this refactor here :)

[LittleFox94]

instead of relying GtS to be built while this test runs, I could add a fallback to use go run as before, would that be ok?

[LittleFox94]

thinking over this while doing laundry things .. this test shouldn't rely on either go run (slow) nor a compiled GtS binary (even if it exists, it could be out of date) - I'll see what's possible :)

[NyaaaWhatsUpDoc]

Oh another note: I'm actually doing this to have nicer test output for my refactoring of config generator stuff - generate way more things. Am quite far with that already, but the test output from the current cliparsing.sh was really slowing things down - hence this refactor here :)

Ooh I'm interested to see this! Do you have a branch somewhere I can take a look at 👀

thinking over this while doing laundry things .. this test shouldn't rely on either go run (slow) nor a compiled GtS binary (even if it exists, it could be out of date) - I'll see what's possible :)

Yeah I was think you might be able to access the necessary exported parts of things in cmd/gotosocial, and update necessary environment / CLI variables by literally calling os.Setenv() and os.Args = []string{"new", "args"}. (under the hood, Go environment variables are loaded once on startup, and depending on the platform setenv() may be called but an in-memory map becomes the main source of them).

[LittleFox94]

Oh another note: I'm actually doing this to have nicer test output for my refactoring of config generator stuff - generate way more things. Am quite far with that already, but the test output from the current cliparsing.sh was really slowing things down - hence this refactor here :)

Ooh I'm interested to see this! Do you have a branch somewhere I can take a look at eyes

not yet, but plan is to have the CLI flags generated from the Configuration struct and that split up for the different groups of CLI flags, the groups split from that embedded in the existing Configuration to have it stay compatible

refactoring internal/config/gen to use templates instead of the current Fprintf stuff and add another template for flags

[NyaaaWhatsUpDoc]

Oh another note: I'm actually doing this to have nicer test output for my refactoring of config generator stuff - generate way more things. Am quite far with that already, but the test output from the current cliparsing.sh was really slowing things down - hence this refactor here :)

Ooh I'm interested to see this! Do you have a branch somewhere I can take a look at eyes

not yet, but plan is to have the CLI flags generated from the Configuration struct and that split up for the different groups of CLI flags, the groups split from that embedded in the existing Configuration to have it stay compatible

refactoring internal/config/gen to use templates instead of the current Fprintf stuff and add another template for flags

This sounds great! My solution was a little hacky at the time, I just didn't like seeing the viper.GetInt(config.SomeConfigKey) everywhere 😅.

As part of this refactor do we think it might be worth splitting up some of the configuration variables we have in the config files themselves? At some point I'd love to add a whole bunch of cache configuration variables and the main config file namespace is getting rather cluttered these days. Obviously one for @tsmethurst to weigh in on too.

[LittleFox94]

As part of this refactor do we think it might be worth splitting up some of the configuration variables we have in the config files themselves? At some point I'd love to add a whole bunch of cache configuration variables and the main config file namespace is getting rather cluttered these days. Obviously one for @tsmethurst to weigh in on too.

yep, my first attempt was to split this up nicely by module, so there would be stuff like this:

type StorageConfiguration struct {
    Backend StorageBackend
    Local   LocalStorageConfiguration
    S3      S3StorageConfiguration
}

and then modules could get their config chunk passed to New() and we wouldn't need any global accessors at all and only give modules the configs they need.

Maybe I'll look at this again, annotating the individual module config structs with a comment for give them a CLI flag prefix could work great ..

//gts:cli-name storage

type StorageConfiguration struct {
    Backend StorageBackend `name:"backend"` // cli name: storage-backend
    Local   LocalStorageConfiguration
    S3      S3StorageConfiguration
}

//gts:cli-name s3

type S3StorageConfiguration struct {
    Endpoint string `name:"endpoint"`  // cli name: storage-s3-endpoint
    UseHTTPS bool   `name:"use_https"` // cli name: storage-s3-use-https ('_' -> '-' automatically)
}

buuut all of this would change config file format I fear... but maybe not, we have the CLI names and could use them ...

Edit: wouldn't even need those gts:cli-name comments if just building the CLI name from config field hierarchy Configuration/Storage/Backend -> storage-backend, Configuration/Storage/S3/Endpoint -> storage-s3-endpoint

[LittleFox94]

I think this looks a lot better now - no go run or running a compiled GtS binary, no extra yaml file for the test cases, way less code so maybe fine without comments now?

[LittleFox94]

maybe I'll also tackle test/envparsing.sh in this PR, could be fitting ... :o)

Buuut ... sleep now 💤

[tsmethurst]

As part of this refactor do we think it might be worth splitting up some of the configuration variables we have in the config files themselves? At some point I'd love to add a whole bunch of cache configuration variables and the main config file namespace is getting rather cluttered these days.

we could, but i'd rather save refactoring work for after 0.6.0 comes out :) especially since people are currently submitting things that involve changing config, and if we change stuff now they have to do difficult rebases

[NyaaaWhatsUpDoc]

As part of this refactor do we think it might be worth splitting up some of the configuration variables we have in the config files themselves? At some point I'd love to add a whole bunch of cache configuration variables and the main config file namespace is getting rather cluttered these days.

we could, but i'd rather save refactoring work for after 0.6.0 comes out :) especially since people are currently submitting things that involve changing config, and if we change stuff now they have to do difficult rebases

'tis a good point!

[LittleFox94]

I'll tackle envparsing.sh in another PR, to get this one merged soon :)

[tsmethurst]

Thank you! :)