Skip to content

HYBIM-952 Rename .galileo dir - #221

Open
ridhima-splunk wants to merge 8 commits into
mainfrom
rename-galileo-dir
Open

HYBIM-952 Rename .galileo dir#221
ridhima-splunk wants to merge 8 commits into
mainfrom
rename-galileo-dir

Conversation

@ridhima-splunk

@ridhima-splunk ridhima-splunk commented Aug 11, 2026

Copy link
Copy Markdown

Overridden directory to .splunk in SplunkAOConfig.
.galileo directory will not be created anymore, only .splunk dir will be created.

Tested with:

uv run python -c "                                                                                                                                                                                                                              
from splunk_ao.config import SplunkAOConfig                                                                                                                                                                                                       
from pathlib import Path                                                                                                                                                                                                                          
                                                                                                                                                                                                                                                    
validated_home = SplunkAOConfig.set_home_dir(Path.home() / '.splunk')                                                                                                                                                                             
print('home_dir:', validated_home)                                                                                                                                                                                                                
print('exists:', validated_home.exists())
"

@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@ridhima-splunk

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

@ridhima-splunk

Copy link
Copy Markdown
Author

recheck

@fercor-cisco fercor-cisco left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 This review was generated by the Astra agent (claude-opus-5). It may contain mistakes.

Verdict: request_changes — The rename works, but the directory name deviates from the one specified in HYBIM-952 (.splunk-ao) without explanation, the new annotation fails the repo's ruff config, the added validator duplicates the inherited one, and nothing locks the new default in with a test.

@fercor-cisco 's note: ignore this comment about the directory name, .splunk is correct.

General Comments

  • 🟡 minor (testing): No test locks in the new default. The sibling rename (HYBIM-918) added test_config_filename_default plus test_config_file_path_resolves_to_splunk_ao_config in tests/test_config.py; this PR adds nothing equivalent, so a future edit to the default (or an accidental loss of the field override when rebasing onto a new galileo-core) silently reverts users to ~/.galileo with no CI signal.

Suggested additions to tests/test_config.py:

def test_home_dir_default_is_dot_splunk() -> None:
    # Assert against Path.home() rather than a monkeypatched HOME: the default is
    # evaluated at import time, so patching HOME in the test has no effect.
    assert SplunkAOConfig.model_fields["home_dir"].default == Path.home() / ".splunk"


def test_set_home_dir_creates_missing_directory(tmp_path) -> None:
    target = tmp_path / "nested" / ".splunk"
    assert SplunkAOConfig.set_home_dir(target) == target
    assert target.is_dir()


def test_set_home_dir_rejects_non_directory(tmp_path) -> None:
    a_file = tmp_path / "not-a-dir"
    a_file.touch()
    with pytest.raises(ValueError):
        SplunkAOConfig.set_home_dir(a_file)
  • 🔵 nit (documentation): The PR description states ".galileo creation will also exist from the parent GalileoConfig". I could not reproduce that: overriding home_dir in SplunkAOConfig replaces the parent field entirely, and galileo_core is the only installed package referencing home_dir — no other GalileoConfig subclass is instantiated by this SDK. Running config construction with HOME pointed at an empty temp dir produces only .splunk, never .galileo.

That's the better outcome (no orphan directory), and the README text is correct as written — but the description should be corrected so reviewers/future readers don't believe both directories are created. If the intent was to cover the case where the legacy galileo SDK is co-installed, say that explicitly.

Follow-ups

Suggested follow-up work that could be tracked as Jira tickets:

  • src/splunk_ao/config.py:75-80: home_dir's default is evaluated at import time (Path.home() / ".splunk"), inherited from galileo-core's pattern. This makes the default insensitive to HOME changes after import, which is awkward for tests and for embedders that set HOME late. A default_factory=lambda: Path.home() / ".splunk" would resolve it lazily at instantiation.
  • splunk-ao-migration-tool/README.md:283-285: The GALILEO_HOME_DIRSPLUNK_AO_HOME_DIR row has the same property that earned GALILEO_API_URL footnote 1: it was never a documented user-facing variable in galileo-python, just an implicit Pydantic settings field on galileo-core's GalileoConfig that becomes an effective rename via _bridge_env_vars(). Consider tagging the new row with footnote 1 as well, so the two implicit-field rows are documented consistently.

Comment thread src/splunk_ao/config.py
Comment thread src/splunk_ao/config.py
Comment thread src/splunk_ao/config.py Outdated
Comment thread src/splunk_ao/config.py
Comment thread src/splunk_ao/config.py
@ridhima-splunk

Copy link
Copy Markdown
Author

@fercor-cisco also resolved general comments/follow-up

@fercor-cisco

fercor-cisco commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

🟡 minor (testing): Would you rather fold one more test into this PR, or track it as a follow-up? It's a single test function, no src/ changes, and it isn't a merge blocker either way — the rest of the branch verifies clean on my side, so this is the last thing I'd flag.

The gap: dropping the _ALL_BRIDGE_PAIRS mirror does kill the drift problem I raised, but it also removes the only independent statement of what _BRIDGE should contain, so the parametrized tests in tests/test_config.py now assert _BRIDGE against itself.

I measured what that costs by deleting two pairs from _BRIDGE in src/splunk_ao/config.py("SPLUNK_AO_SSO_PROVIDER", "GALILEO_SSO_PROVIDER") and the new ("SPLUNK_AO_HOME_DIR", "GALILEO_HOME_DIR") — and running the full suite at 0550ac9c:

  • before: 2058 passed, 8 skipped
  • after: 2054 passed, 8 skippedzero failures

The four parametrized cases (two keys × propagates / does_not_overwrite) simply stopped existing. test_bridge_env_vars_skips_absent_splunk_ao_keys also iterates _BRIDGE, so it shrinks in lockstep and stays green too. Net effect: silently dropping a credential from the bridge — the failure mode where a user sets SPLUNK_AO_API_KEY and auth quietly stops resolving — produces no CI signal, only a smaller test count nobody watches.

That's the inverse of the risk the old mirror had (mirror drift is loud on additions, silent on removals; deriving from _BRIDGE is silent on both), so I'm not sure the swap is a clear win on its own. Cheapest way to get both properties is one assertion that names the pairs independently, with the parametrization still deriving from _BRIDGE:

_EXPECTED_BRIDGE_PAIRS = {
    ("SPLUNK_AO_API_KEY", "GALILEO_API_KEY"),
    ("SPLUNK_AO_API_URL", "GALILEO_API_URL"),
    ("SPLUNK_AO_CONSOLE_URL", "GALILEO_CONSOLE_URL"),
    ("SPLUNK_AO_PROJECT", "GALILEO_PROJECT"),
    ("SPLUNK_AO_PROJECT_ID", "GALILEO_PROJECT_ID"),
    ("SPLUNK_AO_AGENT_STREAM", "GALILEO_LOG_STREAM"),
    ("SPLUNK_AO_LOG_STREAM", "GALILEO_LOG_STREAM"),  # deprecated alias
    ("SPLUNK_AO_AGENT_STREAM_ID", "GALILEO_LOG_STREAM_ID"),
    ("SPLUNK_AO_LOG_STREAM_ID", "GALILEO_LOG_STREAM_ID"),  # deprecated alias
    ("SPLUNK_AO_JWT_TOKEN", "GALILEO_JWT_TOKEN"),
    ("SPLUNK_AO_SSO_ID_TOKEN", "GALILEO_SSO_ID_TOKEN"),
    ("SPLUNK_AO_SSO_PROVIDER", "GALILEO_SSO_PROVIDER"),
    ("SPLUNK_AO_USERNAME", "GALILEO_USERNAME"),
    ("SPLUNK_AO_PASSWORD", "GALILEO_PASSWORD"),
    ("SPLUNK_AO_MODE", "GALILEO_MODE"),
    ("SPLUNK_AO_HOME_DIR", "GALILEO_HOME_DIR"),
}


def test_bridge_pairs_match_expected_set() -> None:
    """Independent statement of the bridge contract: a pair added or removed in
    config.py must be reflected here deliberately, not absorbed silently."""
    assert set(_BRIDGE) == _EXPECTED_BRIDGE_PAIRS

A weaker version (assert len(_BRIDGE) == 16) catches removals but not renames, if the full list reads as too much duplication.

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