More flexible config #126

mmalenic · 2022-12-23T00:58:10Z

Closes #102

Apologies for the very large PR, there is a bunch of changes here.

Changes

Config is now much more flexible, and can configure from environment variables and a TOML config file.
- Envy is swapped out for figment, which is a hierarchical system for config.
- Config files are read using a command line option (--config) or from an environment variable.
- When no command line options are passed, this results in a default configuration with the same behaviour as before this PR.
- Environment variables override config files.
Add multiple bucket support.
Add additional CORS features, like allow origins, methods, headers, and expose headers.
Multiple RegexResolvers are allowed, which can be configured using an array, where the first matching resolver is the one used for the query ID mapping.
- Each resolver can use local storage, or AWS S3 storage.
The resolvers also take into account the query parameters. I.e. It is possible to specify that a given resolver only resolves an ID if the format is Bam, for example.
The local storage data server can be enabled/disabled using a flag in the config file.
Re-done documentation changes for config.
Additional tests for new behaviour.
Various small refactors and re-organisation of code.
A --print-default-config flag can be passed as a command line arg to print the default TOML config to stdout.

Issues

There are still some issues that can be addressed in other PRs.

No support for different file endings yet - i.e. BAM files always must end in .bam, issue Arbitrary file endings for mapped resources #127.
This does increase the complexity of the configuration, however I've attempted to make it as opt-in as possible. Most of the same environment variables are available. The most significant increase in complexity is in the [[resolvers]] array of tables, however this can still be set from an environment variable, and the additional features are not required. The defaults should be the same when running htsget-rs with no configuration.

This PR and branch is also an experiment on using somewhat more conventional commits to see how we feel about it. I haven't used many of the standard types such as feat or bug, but I have added custom types that mostly describe the scope of the change.

Also happy to revert this and squash it as one if we think it's better.

…faults

… and allow methods

…variants

…stom enum

…ve around config options

…figuring cors

…value

… default config

…nable data server

…lexible-config-rename # Conflicts: # htsget-config/README.md # htsget-config/src/config.rs

brainstorm

Woah, that was a beefy PR, sorry it took me a while to go through :-O As you mentioned, try to keep them smaller in the future if you can!

Other than the minor details I commented and running cargo clippy --all-targets --all-features --fix and cargo fmt, I'd advise merging it today instead of later in January as you have this fresh on your head? ;)

Happy xmas and NY otherwise!

brainstorm · 2022-12-23T03:17:03Z

htsget-config/README.md

+| `bucket` | The AWS S3 bucket where resources can be retrieved from. | String | `''` |
+
+Additionally, the resolver component has a feature, which allows resolving IDs based on the other fields present in a query.
+This is useful as allows the resolver to match only match an ID, if a particular set of query parameters are also present. For example, 


*match only an ID

brainstorm · 2022-12-23T03:21:24Z

htsget-config/README.md

+
+The resolvers component of htsget-rs is used to map query IDs to the location of the resource. Each query that htsget-rs receives is
+'resolved' to a location, which a data server can respond with. A query ID is matched with a regex, and is then mapped with a substitution string that
+has access to the regex capture groups. Each resolver is an array of TOML of tables that attempts to match a query ID. This array matches IDs in order, meaning that


Each resolver is an array of TOML of tables

I'd rephrase that :-?

brainstorm · 2022-12-23T03:23:07Z

htsget-config/README.md

+This is useful as allows the resolver to match only match an ID, if a particular set of query parameters are also present. For example, 
+a resolver can be set to only resolve IDs if the format is also BAM.
+
+To set the resolver 'allow guard', add a `[resolver.allow_guard]` table, and set the following options:


What's an "allow guard" resolver?

brainstorm · 2022-12-23T03:26:54Z

htsget-config/README.md

+allow_interval_start = 100
+allow_interval_end = 1000
+```
+


So for example, this resolver would... only allow queries to chr1 between positions 100 and 1000 on BAMs?

Yes, I'll clarify that a bit in the docs.

brainstorm · 2022-12-23T03:27:51Z

htsget-config/README.md

+
+#### Configuring htsget-rs with environment variables
+
+All the htsget-rs config options can be set by environment variables. The ticket server, data server and service info options are flattened and can be set directly using 


All the htsget-rs config options can be set by environment variables

, which is convenient for runtimes such as AWS Lambda

htsget-config/src/config/mod.rs

brainstorm · 2022-12-23T03:34:26Z

htsget-http-lambda/src/lib.rs

@@ -493,7 +485,7 @@ mod tests {
 assert!(router.get_route(&Method::DELETE, &uri).is_none());
 },
 &config,
- formatter_from_config(&config),
+ formatter_from_config(&config).unwrap(),


I'd try to avoid this series of .unwrap() here and below if possible

Yes, this one could definitely be avoided.

Although, this made me think of a different point. All our tests use unwraps to panic, however there is also support to return a Result from a test which could lead to more informative errors and make them more concise. Do you think it would be good for our tests to return a Result? (obviously this is for a different PR, and not super high priority)

Yeah, I tend to prefer avoiding unwrap() even in small examples for that kind of reason. Different issue/PR if that's a big refactor, agreed.

mmalenic · 2022-12-23T03:43:50Z

Thanks for the review! Happy Xmas 😃

How do you feel about the commits and the messages?
This branch is actually cherry-picked from https://github.com/umccr/htsget-rs/tree/more-flexible-config so I could reword and squash commits with a bit more ease.

From now on, I would try and just use a particular commit style so that I don't have to reword things.

brainstorm · 2022-12-23T03:54:22Z

How do you feel about the commits and the messages?

I still haven't had time to review the integrations with conventional commits, but I think that feat(config) might help in categorising the effort we put on fix/feat/chore/docs categories other than just components? Just a thought, no need to change every single commit now, we're just testing the waters and we don't have automation in place... yet ;)

mmalenic · 2022-12-23T04:14:00Z

but I think that feat(config) might help in categorising the effort we put on fix/feat/chore/docs categories other than just components

Yep, that's fair. Also need to do a bit more review with how this works, but would each feat add one minor version bump when the changelog is generated? Or would there only be one increase in the minor version every time a changelog is generated (as long as there is at least one feat), even if there is more than one feat since the last changelog?

…rround environment variables in backticks.

mmalenic added 30 commits December 23, 2022 11:28

config: swap out envy for config dependency

c5d7295

config: add config file from command line or env option

d9f2ad7

config: add separate config for local server and s3 storage

225eb73

config: move fields, tags, no tags, query, and interval to config

2bbfed9

search: add safe cast for conversion between i64 and u64

8c2b3fb

config: implement query matcher logic

a4b03f1

test: add tests for checking for contained value in interval

4a1a777

refactor: move config into separate module

8f1043c

config: use figment instead of config because it is simpler to set de…

505f19e

…faults

refactor: fix errors relating to new config

69ee97a

config: add UrlResolver, separate data server config from resolver

52b7df1

config: add CorsConfig shared struct

bb3875e

config: add cors allow header types for cors config

4f3243c

config: add cors max age option

6a2c177

config: add generic allow type configuration option for allow headers…

2a8927d

… and allow methods

config: add allow origins, and separate out tagged and untagged enum …

8c17cf8

…variants

config: add case insensitive aliases to enum variants

94fc0e4

config: move cors config to separate file

3baab72

config: add expose headers cors option

b450848

config: allow configuring multiple data servers

5c7ad2c

config: remove public fields, add public getters

bc50c7d

config: add cors tests and environment variable tests

f4b4aa4

config: deserialize empty string as None value

85f522b

config: update getter return types

2e2256d

refactor: apply changes to other crates from reworked config

945cc57

config: fix logic involving allowed attributes

7a28706

config: remove custom deserializer for None option and instead use cu…

c0a6e0f

…stom enum

test: fix tests affected by config, change some default values and mo…

fe3283b

…ve around config options

refactor: reduce some options for cors, remove repeated code when con…

bbb33c0

…figuring cors

config: remove setters, add constructors, add documentation.

77a6caa

mmalenic added 11 commits December 23, 2022 11:28

config: allow specifying tags, reference names, fields with an 'All' …

4819bd3

…value

config: update config file with default values, add option to print a…

6b5e60b

… default config

style: clippy and fmt

b1c0ec4

config: flatten data server config

4293a84

docs: add documentation for reworked config

cc87548

bug: fix broken data server optional by introducing boolean flag to e…

c4cc4d1

…nable data server

test: add test for long resolvers from environment variable config

508763e

build: fix feature flag compile errors

5ab9f96

Merge branch 'main' of https://github.com/umccr/htsget-rs into more-f…

c23195b

…lexible-config-rename # Conflicts: # htsget-config/README.md # htsget-config/src/config.rs

bug: remove duplicate config module

963f6e8

docs: reword usage string

fde1f86

mmalenic requested a review from brainstorm December 23, 2022 01:01

mmalenic self-assigned this Dec 23, 2022

mmalenic added the enhancement New feature or request label Dec 23, 2022

brainstorm approved these changes Dec 23, 2022

View reviewed changes

mmalenic added 3 commits December 23, 2022 15:04

docs: clarify how the resolvers work

10d86bf

refactor: remove some unnecessary unwraps

93be5a2

docs: reword resolvers description

512c1f0

mmalenic added 2 commits December 23, 2022 15:22

docs: remove unnecessary default column for environment variables, su…

95ed615

…rround environment variables in backticks.

docs: add missing environment variable options

ad096c8

mmalenic merged commit 5f0ea19 into main Dec 23, 2022

mmalenic deleted the more-flexible-config-rename branch December 23, 2022 05:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More flexible config #126

More flexible config #126

mmalenic commented Dec 23, 2022 •

edited

Loading

brainstorm left a comment

brainstorm Dec 23, 2022

brainstorm Dec 23, 2022

brainstorm Dec 23, 2022

brainstorm Dec 23, 2022

mmalenic Dec 23, 2022

brainstorm Dec 23, 2022

brainstorm Dec 23, 2022

mmalenic Dec 23, 2022 •

edited

Loading

brainstorm Dec 23, 2022 •

edited

Loading

mmalenic commented Dec 23, 2022

brainstorm commented Dec 23, 2022

mmalenic commented Dec 23, 2022 •

edited

Loading


		#### Configuring htsget-rs with environment variables

		All the htsget-rs config options can be set by environment variables. The ticket server, data server and service info options are flattened and can be set directly using

More flexible config #126

More flexible config #126

Conversation

mmalenic commented Dec 23, 2022 • edited Loading

Changes

Issues

brainstorm left a comment

Choose a reason for hiding this comment

brainstorm Dec 23, 2022

Choose a reason for hiding this comment

brainstorm Dec 23, 2022

Choose a reason for hiding this comment

brainstorm Dec 23, 2022

Choose a reason for hiding this comment

brainstorm Dec 23, 2022

Choose a reason for hiding this comment

mmalenic Dec 23, 2022

Choose a reason for hiding this comment

brainstorm Dec 23, 2022

Choose a reason for hiding this comment

brainstorm Dec 23, 2022

Choose a reason for hiding this comment

mmalenic Dec 23, 2022 • edited Loading

Choose a reason for hiding this comment

brainstorm Dec 23, 2022 • edited Loading

Choose a reason for hiding this comment

mmalenic commented Dec 23, 2022

brainstorm commented Dec 23, 2022

mmalenic commented Dec 23, 2022 • edited Loading

mmalenic commented Dec 23, 2022 •

edited

Loading

mmalenic Dec 23, 2022 •

edited

Loading

brainstorm Dec 23, 2022 •

edited

Loading

mmalenic commented Dec 23, 2022 •

edited

Loading