Use ssl

[Jun Aishima (JunAishima)]

• Enable use of SSL to work with updated mx-services system (using nginx to enable https connection)
• Updated tests

[Copilot]

🟡 Changes recommended

Newly added/modified tests currently don’t assert expectations (so they won’t catch regressions), and the use_ssl config should default to a boolean to avoid None semantics.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Enables an SSL/HTTPS mode for the AnalysisClient URL construction to support connecting via an HTTPS-terminating proxy (e.g., nginx), and updates tests/fixtures accordingly.

Changes:

• Add use_ssl configuration to the client (and default config) and switch base URL to https://{host}/ when enabled.
• Update test configuration/fixtures to support SSL-enabled client instances.
• Add a new URL test case intended to validate SSL URL generation.

File summaries

File	Description
analysisstore/client/commands.py	Adds use_ssl handling and HTTPS base URL generation logic.
analysisstore/client/conf.py	Introduces use_ssl default setting.
analysisstore/test/conftest.py	Extends test config and adds an SSL client fixture.
analysisstore/test/test_client.py	Adds SSL URL test case.
analysisstore/test/test_conn_pool.py	Updates bad-config test to include use_ssl.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The SSL behavior introduces unclear/possibly misleading configuration semantics (notably port being ignored under SSL) and the updated tests/fixtures need tightening to reliably detect regressions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

analysisstore/test/test_conn_pool.py:11

• This test no longer asserts anything about the created client (and client is unused), so it won't catch regressions in host/port/SSL configuration. Add explicit assertions for the expected fields.

    config["port"] = testing_config["port"]
    config["use_ssl"] = testing_config["use_ssl"]
    client = AnalysisClient(config)

• Files reviewed: 5/5 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The updated tests/fixtures introduce unused or unverified configuration (notably around SSL client construction), reducing the effectiveness/clarity of the test suite changes.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

analysisstore/test/conftest.py:59

• In astore_client_ssl, port is passed even though AnalysisClient._host_url ignores port when use_ssl=True. Dropping it here avoids implying that non-default HTTPS ports are supported.

analysisstore/test/test_conn_pool.py:11

• test_client_badconf now constructs an AnalysisClient but doesn't assert any behavior (and previously at least compared fields). Adding explicit assertions will keep this test meaningful and ensure use_ssl is propagated as intended.

    config["use_ssl"] = testing_config["use_ssl"]
    client = AnalysisClient(config)

• Files reviewed: 5/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The updated configuration validation error message is misleading and a modified test no longer asserts expected behavior, reducing the chance of catching regressions.

Review details

Suppressed comments (2)

analysisstore/test/test_conn_pool.py:11

• client is created but never asserted on, so this test only checks that construction doesn’t raise and won’t catch regressions in how config is applied. Add assertions for the expected host/port/use_ssl values (or remove the unused variable if the intent is only 'no exception').

    client = AnalysisClient(config)

analysisstore/client/commands.py:19

• The KeyError message is misleading: use_ssl is always present (defaults to False), and the real requirement is that port must be provided when use_ssl is False. Clarifying the message will make configuration errors easier to diagnose.

        if self.port is None and not self.use_ssl:
            raise KeyError("at least one of port or use_ssl must be defined")

• Files reviewed: 6/7 changed files
• Comments generated: 0 new
• Review effort level: Lite