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

Introduce fetching client configuration from a remote JSON file #552

Merged
merged 4 commits into from
Jun 16, 2017

Conversation

oskarhane
Copy link
Member

@oskarhane oskarhane commented May 24, 2017

Intro

This introduces the ability for a client to be configured by passing an URL to the :config command, like: :config https://oskarhane-dropshare-eu.s3-eu-central-1.amazonaws.com/conf-GHrEFakccy/conf.json

If successfully found config params, the :config frame shows up with the new config in it (just as before).
If any error occurs, an error banner shows up below the editor (new feature introduced with styling support).

To discuss

Two questions that comes up are:

  1. Should we restrict fetching hosts to only those in the whitelist? The :style command has no restriction on this.
  2. Should we replace the current config, or merge as we do in the current state of this PR? We replace the :style when fetching a new one. We also replace all params when :params {x:2}.

@akollegger @pe4cey ^^

Error banners

User input error i.e. :config x::
oskar4j 2017-05-24 at 11 46 40

Error in remote json file format:
oskar4j 2017-05-24 at 11 40 32

Error in remote server headers:
oskar4j 2017-05-24 at 11 47 26

Example files that can be used:

(first, add oskarhane-dropshare-eu.s3-eu-central-1.amazonaws.com to your server config browser.remote_content_hostname_whitelist)

Correct formatted file with only new config params:
https://oskarhane-dropshare-eu.s3-eu-central-1.amazonaws.com/conf-eKM9GgLJCo/conf.json

Correct formatted file with existing config params in it:
https://oskarhane-dropshare-eu.s3-eu-central-1.amazonaws.com/conf-GHrEFakccy/conf.json

Invalid formatted file:
https://oskarhane-dropshare-eu.s3-eu-central-1.amazonaws.com/conf2-SX8pcCviMC/conf2.json

Non allowed URL (due to their CORS headers):
http://google.com

@oskarhane
Copy link
Member Author

Decisions made on above points to discuss:

  1. Yes, restrict remote urls to hostnames in the server config browser.remote_content_hostname_whitelist. Also add this restriction to the :style command.
  2. There should be a replace of config, but merged with the default initial config state. That means if you have set maxFrames to 1 and you fetch a remote config that does not include the config maxFrames you should expect it to reset to it's default value. Because that's what it does with the :style and :params commands.

@oskarhane oskarhane force-pushed the 3.0-config-from-url branch from 480c329 to f0db21f Compare May 29, 2017 12:05
@oskarhane
Copy link
Member Author

Updated and ready for review

@pe4cey pe4cey merged commit b37353c into neo4j:3.0 Jun 16, 2017
@oskarhane oskarhane deleted the 3.0-config-from-url branch August 1, 2017 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants