Skip to content

Conversation

@mscoutermarsh
Copy link
Member

For apps where most tables will always be added to a specific keyspace, this makes it possible to set this keyspace via an ENV var PLANETSCALE_DEFAULT_KEYSPACE. Makes the dev workflow a bit easier and the keyspace only needs to be overridden when a table is being added to a keyspace different from the default.

Also, AI'd up some basic test coverage.

For apps where most tables will always be added to a specific keyspace,
this makes it possible to set this keyspace via an ENV var PLANETSCALE_DEFAULT_KEYSPACE. Makes the dev workflow a bit easier and the keyspace only needs to be overridden when a table is being added
to a keyspace different from the default.

Also, AI'd up some basic test coverage.
@nickvanw
Copy link

🛠️ Kit AI Code Review

Priority Issues

Medium Priority

  • Missing validation for keyspace format: The code accepts any non-blank string as a keyspace without validating the format. PlanetScale keyspaces likely have naming conventions that should be enforced. Consider adding validation in lib/planetscale_rails/migration.rb:11.

Low Priority

  • Test framework migration incomplete: The PR switches from RSpec to Minitest but leaves RuboCop configuration that may still reference RSpec patterns. Verify that all tooling configurations are updated consistently.
  • Documentation could be clearer: The README addition at README.md:98-99 mentions the feature is "optional" but doesn't clearly explain the precedence order (explicit keyspace option > ENV var > none).

Summary

This PR adds the ability to set a default keyspace for PlanetScale Rails migrations via the PLANETSCALE_DEFAULT_KEYSPACE environment variable. The key changes include:

  • Core functionality: Modified lib/planetscale_rails/migration.rb:11-12 to check for the ENV var when no explicit keyspace option is provided
  • Test framework migration: Completely switched from RSpec to Minitest, removing spec files and adding comprehensive test coverage
  • Precedence logic: Explicit keyspace option takes priority over the environment variable

The implementation correctly handles the precedence where explicit keyspace options override the environment variable, and both are only applied when ENABLE_PSDB is set.

Recommendations

  1. Add keyspace validation: Consider validating keyspace names against PlanetScale's naming requirements to fail fast on invalid configurations:

    def validate_keyspace(keyspace)
      return unless keyspace.present?
      raise ArgumentError, "Invalid keyspace format" unless keyspace.match?(/\A[a-zA-Z0-9_-]+\z/)
    end
  2. Improve documentation: Expand the README to clearly explain the precedence order and provide examples of both usage patterns:

    The keyspace resolution follows this precedence:
    1. Explicit `keyspace:` option in create_table
    2. `PLANETSCALE_DEFAULT_KEYSPACE` environment variable
    3. No keyspace (standard behavior)
  3. Consider logging: For debugging purposes, consider adding optional logging when the default keyspace is applied, especially in development environments.

  4. Edge case handling: The current implementation handles nil and empty string keyspaces correctly, but consider documenting the expected behavior when PLANETSCALE_DEFAULT_KEYSPACE is set to an empty string.

The test coverage is comprehensive and correctly validates all the key scenarios including precedence rules, environment variable handling, and edge cases. The implementation is solid and follows Rails conventions well.


Generated by cased kit v0.7.1 • Mode: kit • Model: claude-sonnet-4-20250514

@mscoutermarsh mscoutermarsh merged commit 21a0341 into main Jun 10, 2025
6 checks passed
@mscoutermarsh mscoutermarsh deleted the default-keyspace branch June 10, 2025 19:11
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