Skip to content

Conversation

domdomegg
Copy link
Member

Motivation and Context

Cleanup unused configuration, to avoid confusing ourselves in future

How Has This Been Tested?

Everything still runs happily locally, also searched for references

Breaking Changes

None

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

@domdomegg domdomegg marked this pull request as ready for review September 3, 2025 11:07
@domdomegg
Copy link
Member Author

@claude please review

Copy link
Contributor

claude bot commented Sep 3, 2025

Claude finished @domdomegg's task —— View job


Code Review for PR #349

Summary: Clean removal of unused configuration options - looks good to merge!

Review Details

What was removed:

From .env.example:

  • MCP_REGISTRY_LOG_LEVEL=info
  • MCP_REGISTRY_DATABASE_NAME=mcp-registry
  • MCP_REGISTRY_COLLECTION_NAME=servers_v2

From internal/config/config.go:

  • DatabaseName string field (line ~20)
  • CollectionName string field (line ~21)
  • LogLevel string field (line ~22)

Analysis:

Verification Complete: Searched the entire codebase and confirmed these configuration options are not referenced anywhere in the application code.

Clean Removal: The struct fields and environment variables were removed consistently from both the config definition and the example file.

No Breaking Changes: Since these were unused, removal doesn't break any functionality.

Notes:

  • Found some references to COLLECTION_NAME and LOG_LEVEL in /data/seed.json and /docs/server-json/examples.md, but these are examples for external MCP servers, not the registry's own configuration
  • The GF_DATABASE_NAME reference in deploy/pkg/k8s/monitoring.go is unrelated (Grafana configuration)
  • This cleanup will help prevent confusion about which config options are actually used

Recommendation:

✅ LGTM - This is exactly the kind of cleanup that keeps codebases maintainable. Safe to merge.


Copy link
Contributor

@rdimitrov rdimitrov left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@domdomegg domdomegg merged commit 6ea3828 into main Sep 3, 2025
6 checks passed
@domdomegg domdomegg deleted the adamj/unused-config branch September 3, 2025 15:02
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