TPT-4130: Add Valkey Managed Database integration tests by anmsharm1 · Pull Request #932 · linode/linode-cli

anmsharm1 · 2026-10-13T18:43:00Z

📝 Description

What does this PR do and why is this change necessary?

Adds integration test coverage for Valkey Managed Databases, including engine and type discovery, database listing and viewing, engine configuration, credentials and SSL, suspend/resume, patching, and snapshot-based fork/restore.

Adds table output handling for valkey-config-view, including readable formatting for nullable configuration parameter types ( backup_minute, backup_hour, valkey_memory_policy etc. ), such as integer | null or string | null. This is required as Valkey’s config schema includes nullable parameter types represented as lists, such as ["integer", "null"] .

The new formatting helper renders those as integer | null. I modeled add_param_row that now applies that formatting only when the Valkey handler opts in otherwise its default preserves the existing type formatting for PostgreSQL and MySQL.

✔️ How to Test

What are the steps to reproduce the issue or verify the changes?

Set LINODE_CLI_TOKEN to a token for an account that can create Managed Databases. The integration tests create and delete live database resources. Snapshot restore is skipped by default; set RUN_LONG_TESTS=True to run it.

How do I run the relevant unit/integration tests?

make test-unit — passed.
make test-int TEST_SUITE=database TEST_CASE=valkey TEST_ARGS='-v' — 10 passed, 1 skipped, 17 deselected.

Render nullable types as a readable union in database configuration tables. Cover Valkey output without changing non-nullable types. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

akasklock · 2026-10-22T18:28:08Z



-def add_param_row(output, param_name, param_data):
+def _format_param_type(param_type) -> str:


Nit: not sure how consistently the convention is followed in this repo, but in this module at least it looks like the step-down rule more-or-less applies, which means this should follow add_param_row().

akasklock · 2026-10-22T22:15:09Z

+        }
+
+        with patch("linodecli.overrides.Console", return_value=MockConsole()):
+            assert databases_valkey_config_view_output(response_json) is False


Nit: assert not databases_valkey_config_view_output(response_json)? (I do see is False assertions elsewhere in the repo, but I gather they're intended specifically to verify that the human-facing output from the tool is actually False rather than merely something falsy, which is a distinction that I suspect does not matter for this function.)

akasklock · 2026-10-22T22:28:18Z



-def add_param_row(output, param_name, param_data):
+def _format_param_type(param_type) -> str:


It looks like this is intentional, but it's worth noting in this context that the logic here makes the behavior for Valkey parameters with multiple types different from the other engines. I can't immediately produce an example for PostgreSQL, which is currently broken in the CLI (!), but I can for MySQL:

├────────────────────────────────┼────────────────────┼────────────────────┼─────────┼──────────────────────┼────────────┼────────────┼───────────────────────┼──────────────────┼────────────────────────────────┤ │ mysql.innodb_ft_server_stopwo… │ ['null', 'string'] │ db_name/table_name │ │ │ │ 1024 │ ^.+/.+$ │ NO │ This option is used to specify │ │ │ │ │ │ │ │ │ │ │ your own InnoDB FULLTEXT index │ │ │ │ │ │ │ │ │ │ │ stopword list for all InnoDB │ │ │ │ │ │ │ │ │ │ │ tables. │ ├────────────────────────────────┼────────────────────┼────────────────────┼─────────┼──────────────────────┼────────────┼────────────┼───────────────────────┼──────────────────┼────────────────────────────────┤

... where the type of the parameter would be given as null | string for Valkey.

Maybe this is a question best addressed to the maintainers, but: what's the right thing to do here? Aim for consistency across the engine types? If so, should MySQL + (presumably) PG switch to the formatting applied here, or should Valkey use the existing formatting?

akasklock · 2026-10-22T22:46:31Z

+        )
+        assert isinstance(config[key]["requires_restart"], bool)
+
+    assert config["backup_hour"]["minimum"] == 0


Do we really want to assert on the values of these fields? I think it's reasonable to expect that the config fields available at launch will be available indefinitely (as tossing any would be breaking), but I don't think it's safe to assume that min, max, requires_restart, etc. values won't change over time. We don't want these tests to break because, e.g., Aiven decided to widen the permitted range for one of them. (Maybe similar goes for the expected types above, at least inasmuch as some that may not currently be nullable may become nullable in the future.)

akasklock · 2026-10-22T22:50:07Z

+        )
+
+    wait_for_condition(15, 900, config_matches)
+    valkey_cluster["engine_config"].update(updated_config)


If the wait above fails, this line won't execute. Is that okay, or might it cause additional test failures?

akasklock · 2026-10-22T22:57:18Z

+            + ["valkey-resume", database_id, "--text", "--delimiter=,"]
+        )
+        assert "Request failed: 400" not in res
+        wait_for_database_status("valkey", database_id, "active")


This added step for Valkey will add on the order of minutes of latency relative to the MySQL/PG tests, likely similar to the added cost of forking... which suggests that the RUNG_LONG_TESTS guard might be applicable here. (Do we know what the threshold is beyond which it should be applied?)

akasklock · 2026-10-22T22:59:55Z


+def test_valkey_suspend_resume(valkey_cluster):
+    database_id = str(valkey_cluster["id"])
+    wait_for_database_status("valkey", database_id, "active")


FWIW, it feels a little odd to wait for active status both at the start and the end of the tests. I'd naively expect either that tests that need the instance in active status would consistently wait for that at the start or that tests moving the database out of active would consistently wait for that, e.g., at teardown. (Which ultimately sounds like a fixture to me, I think.)

akasklock · 2026-10-23T00:25:50Z

+
+
+@pytest.fixture(scope="session")
+def valkey_cluster():


The approach in valkey_cluster() feels like an improvement to me relative to the other database engines -- being flexible about the type, region, and engine; making sure the instance is ready before use; and sharing more details with the tests that consume the instance all seem like facially good, reasonable choices -- but there are tradeoffs that are worth explicitly considering, I think.

One is whether we want to leave the CLI in a state where Valkey is treated very differently from PostgreSQL and MySQL. That's potentially both a maintainability issue and a source of inconsistency in the quality of the products. A plausible answer to that is to line up PG and MySQL with Valkey, though -- either in this PR or in a followup -- rather than to move in the other direction.

Another is how comprehensive these tests are intended to be/should be. Some of the testing here arguably veers into testing the Managed Database functionality itself, which might be more than we want from these tests. The additional complexity and coverage here carries some maintenance burden that's worth taking into account. Probably this question is mostly for the maintainers.

Somewhat relatedly: some of these steps will nontrivially increase the running time of the tests. Probably not a huge deal, but FWIW it may mean we should be more aggressive about treating these tests as long-running.

anmsharm1 and others added 6 commits September 29, 2026 12:17

DBAAS1-ValkeyCLI

fd7cc7d

Fix nullable database config type display

c1b63e9

Render nullable types as a readable union in database configuration tables. Cover Valkey output without changing non-nullable types. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

test valkey integ

496f407

DBAAS1-1774: adjust code to limit refactor

7612779

Merge branch 'dev' into DBAAS1-1774

b84c15a

Merge branch 'linode:dev' into DBAAS1-1774

f914f8d

lgarber-akamai requested review from akasklock and dchavan25 October 15, 2026 17:04

akasklock reviewed Oct 23, 2026

View reviewed changes

Navigation Menu

Repository navigation

TPT-4130: Add Valkey Managed Database integration tests - #932

TPT-4130: Add Valkey Managed Database integration tests#932
anmsharm1 wants to merge 6 commits into
linode:devfrom
anmsharm1:DBAAS1-1774

anmsharm1 commented Oct 13, 2026

Uh oh!

akasklock Oct 22, 2026

Uh oh!

akasklock Oct 22, 2026

Uh oh!

akasklock Oct 22, 2026

Uh oh!

akasklock Oct 22, 2026

Uh oh!

akasklock Oct 22, 2026

Uh oh!

akasklock Oct 22, 2026

Uh oh!

akasklock Oct 22, 2026

Uh oh!

akasklock Oct 23, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants



		def add_param_row(output, param_name, param_data):
		def _format_param_type(param_type) -> str:

Conversation

anmsharm1 commented Oct 13, 2026

📝 Description

✔️ How to Test

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants