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

datasette.yaml plugin support #2183

Merged
merged 8 commits into from
Sep 13, 2023
Merged

Conversation

asg017
Copy link
Collaborator

@asg017 asg017 commented Sep 11, 2023

Part of #2093

In #2149 , we ported over "settings.json" into the new datasette.yaml config file, with a top-level "settings" key. This PR ports over plugin configuration into top-level "plugins" key, as well as nested database/table plugin config.

From now on, no plugin-related configuration is allowed in metadata.yaml, and must be in datasette.yaml in this new format. This is a pretty significant breaking change. Thankfully, you should be able to copy-paste your legacy plugin key/values into the new datasette.yaml format.

An example of what datasette.yaml would look like with this new plugin config:

plugins:
  datasette-my-plugin:
    config_key: value

databases:
  fixtures:
      plugins: 
        datasette-my-plugin:
          config_key: fixtures-db-value
    tables:
      students:
        plugins:
          datasette-my-plugin:
            config_key: fixtures-students-table-value

As an additional benefit, this now works with the new -s flag:

datasette --memory -s 'plugins.datasette-my-plugin.config_key' new_value

Marked as a "Draft" right now until I add better documentation. We also should have a plan for the next alpha release to document and publicize this change, especially for plugin authors (since their docs will have to change to say datasette.yaml instead of metadata.yaml


📚 Documentation preview 📚: https://datasette--2183.org.readthedocs.build/en/2183/

@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Patch coverage: 95.00% and project coverage change: -0.04% ⚠️

Comparison is base (a4c96d0) 92.69% compared to head (659dcbd) 92.66%.

❗ Current head 659dcbd differs from pull request most recent head acca338. Consider uploading reports for the commit acca338 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2183      +/-   ##
==========================================
- Coverage   92.69%   92.66%   -0.04%     
==========================================
  Files          40       40              
  Lines        6025     6039      +14     
==========================================
+ Hits         5585     5596      +11     
- Misses        440      443       +3     
Files Changed Coverage Δ
datasette/app.py 94.19% <95.00%> (-0.24%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@simonw
Copy link
Owner

simonw commented Sep 11, 2023

On thinking about this further, I'm fine releasing it as another alpha provided it causes Datasette to error loudly with an explanatory message if you attempt to load -m metadata.json at a metadata file that includes "plugins" configuration.

Something like this:

datasette -m metadata.json

Outputs:

Datasette no longer accepts plugin configuration in --metadata. Move your "plugins" configuration blocks to a separate file - we suggest calling that datasette.yaml - and start Datasette with datasette -c datasette.yaml.

For added usability points, let's have it suggest datasette.json if they used metadata.json and datasette.yaml if they tried to use metadata.yaml.

@asg017 asg017 marked this pull request as ready for review September 13, 2023 01:30
@asg017
Copy link
Collaborator Author

asg017 commented Sep 13, 2023

@simonw docs are finished, this is ready for review!

One thing: I added "Configuration" as a top-level item in the documentation site, at the very bottom. Not sure if this is the best, maybe it can be named "datasette.yaml Configuration" or something similar?

Mostly because "Configuration" by itself can mean many things, but adding "datasette.yaml" would make it pretty clear it's about that specific file, and is easier to scan. I'd also be fine with using "datasette.yaml" instead of "datasette.json", since writing in YAML is much more forgiving (and advanced users will know JSON is also supported)

Also, maybe this is a chance to consolidate the docs a bit? I think "Settings", "Configuration", "Metadata", and "Authentication and permissions" should possibly be under the same section. Maybe even consolidate the different Plugin pages that exist?

@@ -782,7 +798,7 @@ def assert_permissions_checked(datasette, actions):
type=click.Path(file_okay=True, dir_okay=False),
help="Write out second test DB to this file",
)
def cli(db_filename, metadata, plugins_path, recreate, extra_db_filename):
Copy link
Owner

Choose a reason for hiding this comment

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

I like that you updated this too. Trying that myself:

python tests/fixtures.py --help
Usage: fixtures.py [OPTIONS] [DB_FILENAME] [METADATA] [CONFIG] [PLUGINS_PATH]

  Write out the fixtures database used by Datasette's test suite

Options:
  --recreate                Delete and recreate database if it exists
  --extra-db-filename FILE  Write out second test DB to this file
  --help                    Show this message and exit.
python tests/fixtures.py /tmp/demo/fixtures.db /tmp/demo/metadata.json /tmp/demo/config.json /tmp/demo/plugins
Test tables written to /tmp/demo/fixtures.db
- metadata written to /tmp/demo/metadata.json
- config written to /tmp/demo/config.json
  Wrote plugin: /tmp/demo/plugins/register_output_renderer.py
  Wrote plugin: /tmp/demo/plugins/view_name.py
  Wrote plugin: /tmp/demo/plugins/my_plugin.py
  Wrote plugin: /tmp/demo/plugins/messages_output_renderer.py
  Wrote plugin: /tmp/demo/plugins/sleep_sql_function.py
  Wrote plugin: /tmp/demo/plugins/my_plugin_2.py
find /tmp/demo
/tmp/demo
/tmp/demo/plugins
/tmp/demo/plugins/register_output_renderer.py
/tmp/demo/plugins/view_name.py
/tmp/demo/plugins/my_plugin.py
/tmp/demo/plugins/messages_output_renderer.py
/tmp/demo/plugins/sleep_sql_function.py
/tmp/demo/plugins/my_plugin_2.py
/tmp/demo/config.json
/tmp/demo/fixtures.db
/tmp/demo/metadata.json
cat /tmp/demo/config.json
{
    "plugins": {
        "name-of-plugin": {
            "depth": "root"
        },
        "env-plugin": {
            "foo": {
                "$env": "FOO_ENV"
            }
        },
        "env-plugin-list": [
            {
                "in_a_list": {
                    "$env": "FOO_ENV"
                }
            }
        ],
        "file-plugin": {
            "foo": {
                "$file": "/var/folders/x6/31xf1vxj0nn9mxqq8z0mmcfw0000gn/T/plugin-secret"
            }
        }
    },
    "databases": {
        "fixtures": {
            "plugins": {
                "name-of-plugin": {
                    "depth": "database"
                }
            },
            "tables": {
                "simple_primary_key": {
                    "plugins": {
                        "name-of-plugin": {
                            "depth": "table",
                            "special": "this-is-simple_primary_key"
                        }
                    }
                },
                "sortable": {
                    "plugins": {
                        "name-of-plugin": {
                            "depth": "table"
                        }
                    }
                }
            }
        }
    }
}

)
assert result.exit_code == 0, result.output
assert (
json.loads(result.output).get("rows")[0].get("prepare_connection_args()")
Copy link
Owner

Choose a reason for hiding this comment

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

I couldn't remember how this worked, found the answer here:

@hookimpl
def prepare_connection(conn, database, datasette):
def convert_units(amount, from_, to_):
"""select convert_units(100, 'm', 'ft');"""
return (amount * ureg(from_)).to(to_).to_tuple()[0]
conn.create_function("convert_units", 3, convert_units)
def prepare_connection_args():
return 'database={}, datasette.plugin_config("name-of-plugin")={}'.format(
database, datasette.plugin_config("name-of-plugin")
)
conn.create_function("prepare_connection_args", 0, prepare_connection_args)

Comment on lines -249 to -253
# Ensure secrets aren't visible in /-/metadata.json
metadata = await ds_client.get("/-/metadata.json")
assert [{"in_a_list": {"$env": "FOO_ENV"}}] == metadata.json()["plugins"][
"env-plugin-list"
]
Copy link
Owner

Choose a reason for hiding this comment

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

This made me think. That {"$env": "ENV_VAR"} hack was introduced back here:

The problem it was solving was that metadata was visible to everyone with access to the instance at /-/metadata but plugins clearly needed a way to set secret settings.

Now that this stuff is moving to config, we have some decisions to make:

  1. Add /-/config to let people see the configuration of their instance, and keep the $env trick for secret settings.
  2. Say all configuration aside from metadata is secret and make $env optional or ditch it entirely.
  3. Allow plugins to announce which of their configuration options are secret so we can automatically redact them from /-/config

I've found /-/metadata extraordinarily useful as a user of Datasette - it really helps me understand exactly what's going on if I run into any problems with a plugin, if I can quickly check what the settings look like.

So I'm leaning towards option 1 or 3.

@simonw
Copy link
Owner

simonw commented Sep 13, 2023

I'm going to land this and make any further documentation tweaks on main.

@simonw simonw merged commit b2ec871 into simonw:main Sep 13, 2023
8 checks passed
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