Skip to content

Conversation

@bessey
Copy link
Contributor

@bessey bessey commented Apr 4, 2023

What is the purpose of this pull request?

Ensures that EJSON, like the ENV and Doppler loaders, supports not utilising the namespaced configuration feature.

Happy to follow up with tests and docs if you think this is reasonable!

Is there anything you'd like reviewers to focus on?

To keep the API surface small I have used the env_prefix keyword, even though its not really a prefix in this context (JSON nested keys). I figure consistency is preferable.

Checklist

  • I've added tests for this change
  • I've added a Changelog entry
  • I've updated a documentation

This is supported by the env and doppler loaders. EJSON is the odd one out
@palkan
Copy link
Owner

palkan commented Apr 5, 2023

Thanks for the proposal!

Agree, it makes. Though I'm not sure env_prefix is the right way to handle this.

I think, we should introduce an API to pass custom options to loaders. Smth like:

class MyConfig < Anyway::Config
  loader_options ejson_prefix: "" 
end

@bessey bessey changed the title Add env_prefix "" support to EJSON Add loader_options, use to add ejson_namespace nil support Apr 5, 2023
@bessey
Copy link
Contributor Author

bessey commented Apr 5, 2023

Hey @palkan, yeah I prefer that, certainly more modular for those adding their own loaders.

I went with ejson_namespace as I felt "namespace" more accurately describes the concept in JSON. "prefix" makes sense when its string concatenation, but here we are nesting JSON blobs within X. X = a namespace.

Introduced some low level tests but I wasn't sure how best to write tests for Config since right now only the EJSON loader uses this functionality.

I also used nil to signify "no namespace" but I think perhaps false would be a better option actually. I know prefix uses "" but again, that doesn't seem appropriate in this context to me. Its a small change though so if that consistency is preferred, its easily done.

If you want to take this PR over at this point, feel free to make your own changes.

Copy link
Owner

@palkan palkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM.

Please, take a look at the comments and CI failures.

end

it "parses default EJSON" do
# It also includes the public key, but we don't care about it
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, it makes sense to remove "_public_key" from the resulting hash to avoid it's potential leakage?


return @loader_options if instance_variable_defined?(:@loader_options)

@loader_options = {}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, we also need to take care of inheritance (see other class-level configuration methods)

next unless secrets_hash

config_hash = secrets_hash[name]
config_hash = if ejson_namespace.nil?
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think perhaps false would be a better option actually.

Agree, false sounds better. Let's just replace the check with unless ejson_namespace to handle both nil and false

@bessey bessey requested a review from palkan April 11, 2023 10:14
@bessey
Copy link
Contributor Author

bessey commented Apr 11, 2023

All changes made and hopefully CI passes now (I don't have JRuby to test against so somewhat coding blind there)

@palkan
Copy link
Owner

palkan commented Apr 11, 2023

Thanks! I'll take care of linting issues.

@palkan palkan merged commit fe07d0b into palkan:master Apr 11, 2023
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.

2 participants