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

Hide extract_headers_path, extract_headers_ttl #699

Merged
merged 1 commit into from Mar 6, 2019
Merged

Conversation

kishansagathiya
Copy link
Contributor

proxy: "extract_headers_path" and "extract_headers_ttl" should be
hidden options in the config

Fixes #698

@ghost ghost assigned kishansagathiya Mar 5, 2019
@ghost ghost added the status/in-progress In progress label Mar 5, 2019
Copy link
Collaborator

@hsanjuan hsanjuan left a comment

Choose a reason for hiding this comment

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

The usual trick to do this plays only in the toJSON and fromJSON sides.

From JSON does a Default() first so Config will have the values correctly set. If the values in JSON for these fields are the type-default ("" or 0), then the Config should not be overwritten (SetIfNotDefault takes care of this).

In order to hide these values when they are the default, ToJSON sets "" when the value of Config is the default.

Note that, with this change, Config.ExtractHeadersPath will be unset and this will break. We expect Config to always have a valid value for those fields. We just want to hide the fields in the json by omitting them when they are the default.

api/ipfsproxy/config.go Outdated Show resolved Hide resolved
@@ -116,8 +114,6 @@ func (cfg *Config) Default() error {
cfg.WriteTimeout = DefaultWriteTimeout
cfg.IdleTimeout = DefaultIdleTimeout
cfg.ExtractHeadersExtra = nil
cfg.ExtractHeadersPath = DefaultExtractHeadersPath
cfg.ExtractHeadersTTL = DefaultExtractHeadersTTL
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep this

api/ipfsproxy/config.go Outdated Show resolved Hide resolved
api/ipfsproxy/config.go Outdated Show resolved Hide resolved
@@ -259,6 +251,9 @@ func (cfg *Config) toJSONConfig() (jcfg *jsonConfig, err error) {

jcfg.ExtractHeadersExtra = cfg.ExtractHeadersExtra
jcfg.ExtractHeadersPath = cfg.ExtractHeadersPath
if !(cfg.ExtractHeadersTTL > 0) {
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

returning here breaks the flow of the function. If we were to add things at the end of this function we would need to remember that the function may have been aborted before.

api/ipfsproxy/config_test.go Outdated Show resolved Hide resolved
@@ -250,8 +258,14 @@ func (cfg *Config) toJSONConfig() (jcfg *jsonConfig, err error) {
jcfg.NodeHTTPS = cfg.NodeHTTPS

jcfg.ExtractHeadersExtra = cfg.ExtractHeadersExtra
jcfg.ExtractHeadersPath = cfg.ExtractHeadersPath
if !(cfg.ExtractHeadersTTL > 0) {
if cfg.ExtractHeadersPath == DefaultExtractHeadersPath {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The first case is not necessary because the json object is empty. So the if should only have the second case.

}

if cfg.ExtractHeadersTTL == DefaultExtractHeadersTTL {
jcfg.ExtractHeadersTTL = ""
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as before. Keep consistency.

if ttl := cfg.ExtractHeadersTTL; ttl != DefaultExtractHeadersTTL { jcfg.ExtractHeadersTTL = ttl.String() }

do not return

proxy: "extract_headers_path" and "extract_headers_ttl" should be
hidden options in the config
Copy link
Collaborator

@hsanjuan hsanjuan left a comment

Choose a reason for hiding this comment

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

Thanks!

@hsanjuan hsanjuan merged commit 9fedd6b into master Mar 6, 2019
@hsanjuan hsanjuan deleted the issue_698 branch March 6, 2019 18:40
@ghost ghost removed the status/in-progress In progress label Mar 6, 2019
@hsanjuan hsanjuan mentioned this pull request Mar 6, 2019
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