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

Remove backwards compatibility hacks #682

Merged
merged 2 commits into from Feb 21, 2019

Conversation

@hsanjuan
Copy link
Collaborator

commented Feb 20, 2019

The things removed here have been live for more than 2 releases.

License: MIT
Signed-off-by: Hector Sanjuan hector@protocol.ai

@hsanjuan hsanjuan requested review from lanzafame and kishansagathiya Feb 20, 2019
@ghost ghost assigned hsanjuan Feb 20, 2019
@ghost ghost added the in progress label Feb 20, 2019
MonitorPingInterval string `json:"monitor_ping_interval"`
PeerWatchInterval string `json:"peer_watch_interval"`
DisableRepinning bool `json:"disable_repinning"`
PeerstoreFile string `json:"peerstore_file,omitempty"`

This comment has been minimized.

Copy link
@hsanjuan

hsanjuan Feb 20, 2019

Author Collaborator

Removed Peers, Bootstrap and ReplicationFactor.

@coveralls

This comment has been minimized.

Copy link

commented Feb 20, 2019

Coverage Status

Coverage increased (+0.2%) to 65.101% when pulling 0fed611 on feat/remove-legacy into f57c5e4 on master.

The things removed here have been live for more than 2 releases.

License: MIT
Signed-off-by: Hector Sanjuan <hector@protocol.ai>
@hsanjuan hsanjuan force-pushed the feat/remove-legacy branch from 546f70d to 0fed611 Feb 20, 2019
@hsanjuan hsanjuan referenced this pull request Feb 20, 2019
ProxyReadTimeout string `json:"proxy_read_timeout,omitempty"`
ProxyReadHeaderTimeout string `json:"proxy_read_header_timeout,omitempty"`
ProxyWriteTimeout string `json:"proxy_write_timeout,omitempty"`
ProxyIdleTimeout string `json:"proxy_idle_timeout,omitempty"`

This comment has been minimized.

Copy link
@kishansagathiya

kishansagathiya Feb 20, 2019

Member

This would trigger removing things from ipfsproxy's config.go

// Below fields are only here to maintain backward compatibility
// They will be removed in future
ProxyListenMultiaddress string `json:"proxy_listen_multiaddress,omitempty"`
ProxyReadTimeout string `json:"proxy_read_timeout,omitempty"`
ProxyReadHeaderTimeout string `json:"proxy_read_header_timeout,omitempty"`
ProxyWriteTimeout string `json:"proxy_write_timeout,omitempty"`
ProxyIdleTimeout string `json:"proxy_idle_timeout,omitempty"`

func (jcfg *jsonConfig) toNewFields() {
if jcfg.ListenMultiaddress == "" {
jcfg.ListenMultiaddress = jcfg.ProxyListenMultiaddress
}
if jcfg.ReadTimeout == "" {
jcfg.ReadTimeout = jcfg.ProxyReadTimeout
}
if jcfg.ReadHeaderTimeout == "" {
jcfg.ReadHeaderTimeout = jcfg.ProxyReadHeaderTimeout
}
if jcfg.WriteTimeout == "" {
jcfg.WriteTimeout = jcfg.ProxyWriteTimeout
}
if jcfg.IdleTimeout == "" {
jcfg.IdleTimeout = jcfg.ProxyIdleTimeout
}
}

// This is here only here to maintain backward compatibility
// This won't be needed after removing old style fields(starting with `proxy_`)
jcfg.toNewFields()

and this check would not be required, including the warning

// Should we change hardcoded "ipfsproxy" to something else
if _, ok := jcfg.API["ipfsproxy"]; !ok {
loadCompJSON("ipfshttp", sections[API]["ipfsproxy"], jcfg.IPFSConn)
logger.Warning(`
The IPFS proxy functionality has been extracted as a separate component
and now uses its own configuration section ("ipfsproxy" in the "api" section).
To keep compatibility, since you did not define an "ipfsproxy" section, the
proxy configuration is taken from the "ipfshttp" section as before, but this
will be removed in future versions.
`)

This comment has been minimized.

Copy link
@hsanjuan

hsanjuan Feb 20, 2019

Author Collaborator

Thanks, should be addressed now.

License: MIT
Signed-off-by: Hector Sanjuan <hector@protocol.ai>
Copy link
Member

left a comment

Missed this one

// Should we change hardcoded "ipfsproxy" to something else
if _, ok := jcfg.API["ipfsproxy"]; !ok {
loadCompJSON("ipfshttp", sections[API]["ipfsproxy"], jcfg.IPFSConn)
logger.Warning(`
The IPFS proxy functionality has been extracted as a separate component
and now uses its own configuration section ("ipfsproxy" in the "api" section).
To keep compatibility, since you did not define an "ipfsproxy" section, the
proxy configuration is taken from the "ipfshttp" section as before, but this
will be removed in future versions.
`)

@kishansagathiya kishansagathiya dismissed their stale review Feb 21, 2019

Already done in this commit 0fed611

@hsanjuan hsanjuan merged commit add08da into master Feb 21, 2019
5 of 6 checks passed
5 of 6 checks passed
continuous-integration/jenkins/pr-merge This commit cannot be built
Details
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
codeclimate All good!
Details
commit-message-check/gitcop All commit messages are valid
Details
coverage/coveralls Coverage increased (+0.06%) to 65.116%
Details
@ghost ghost removed the in progress label Feb 21, 2019
@hsanjuan hsanjuan deleted the feat/remove-legacy branch Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.