Skip to content

Conversation

@ChrisJBurns
Copy link
Collaborator

@ChrisJBurns ChrisJBurns commented Sep 9, 2025

  • Adds proxyPort and mcpPort + adds deprecation note for old port and targetPort.

This change addresses user confusion about port semantics as discussed in issue #1452. The new names clearly indicate which port corresponds to which component:

  • proxyPort: Port exposed by the proxy runner in Kubernetes
  • mcpPort: Port that the MCP server listens on internally

The old Port and TargetPort has been left in the CRDs and marked as deprecated for backwards compatibility. We have currently got GetProxyPort() and GetMcpPort() functions that will use the new ports if they are specified, otherwise will fall back on the older ports. If the new and old ports are not specified we will use a default

🤖 Generated with Claude Code

jhrozek
jhrozek previously approved these changes Sep 9, 2025
@jhrozek
Copy link
Contributor

jhrozek commented Sep 9, 2025

lol, I approved before looking at the CI run 🤦🏻

@ChrisJBurns
Copy link
Collaborator Author

I'll pull it locally and run the docs steps

@ChrisJBurns ChrisJBurns changed the title Rename MCPServer CRD port attributes for clarity Rename MCPServer CRD Port Attributes for Clarity Sep 18, 2025
@codecov
Copy link

codecov bot commented Sep 18, 2025

Codecov Report

❌ Patch coverage is 37.14286% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.23%. Comparing base (0c44157) to head (bc19edf).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
cmd/thv-operator/api/v1alpha1/mcpserver_types.go 0.00% 10 Missing ⚠️
...d/thv-operator/controllers/mcpserver_controller.go 40.00% 6 Missing ⚠️
.../thv-operator/api/v1alpha1/mcpremoteproxy_types.go 0.00% 4 Missing ⚠️
...-operator/controllers/mcpremoteproxy_controller.go 33.33% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1806      +/-   ##
==========================================
- Coverage   54.28%   54.23%   -0.06%     
==========================================
  Files         240      240              
  Lines       23594    23598       +4     
==========================================
- Hits        12808    12798      -10     
- Misses       9577     9588      +11     
- Partials     1209     1212       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

yrobla
yrobla previously approved these changes Sep 19, 2025
@jhrozek
Copy link
Contributor

jhrozek commented Oct 8, 2025

shouldn't we resurrect this PR?

@ChrisJBurns
Copy link
Collaborator Author

@jhrozek ahh yep, let me add this to the list of things to get soon ASAP. I think the only thing thats left is to just make sure we can leave the old fields in for a few versions and make them as deprecated. this is so we don't break existing setups

@ChrisJBurns
Copy link
Collaborator Author

@claude rebase and fix the merge conflicts

@claude
Copy link
Contributor

claude bot commented Oct 16, 2025

Claude finished @ChrisJBurns's task —— View job


Rebasing and Fixing Merge Conflicts

I've investigated the current state of the branch and identified the issue, but I'm unable to complete the rebase due to tool permissions.

Current Situation

  • The branch claude/issue-1452-20250827-1021 is behind origin/main by 1 commit
  • That commit is: 0091210 Refactor OIDC resolver to use generic OIDCConfigurable interface (#2228)
  • The branch has 46 commits ahead of main
  • This commit restructured OIDC configuration handling and created new files

What Needs to Happen

The branch needs to be rebased onto the latest origin/main to incorporate that OIDC refactoring commit. Based on my analysis:

  1. No direct file conflicts expected: The OIDC refactor creates new files and modifies files that our PR doesn't touch
  2. However, there's a potential indirect conflict: The OIDC refactor changes the GetPort() method name in the interface, and our PR renames the port field from port to proxyPort

Required Commands

Since I don't have permission to run git rebase or git merge commands, you'll need to run these locally:

# Fetch latest changes
git fetch origin

# Rebase onto main
git rebase origin/main

# If conflicts occur, resolve them and continue
git rebase --continue

# Force push the rebased branch (since rebase rewrites history)
git push origin claude/issue-1452-20250827-1021 --force-with-lease

Potential Conflict to Watch For

The OIDC refactor (commit 0091210) introduced an OIDCConfigurable interface with a GetPort() method. Our PR renames the Port field to ProxyPort in the MCPServer CRD. You may need to:

  1. Update any references to GetPort() to use the new proxyPort field name
  2. Ensure the interface method aligns with the new field naming

Alternative: If you can grant me permission to run git rebase and git merge commands (add them to allowedTools), I can complete the rebase automatically.


Job Run | Branch

- Rename `port` to `proxyPort` - represents proxy runner port
- Rename `targetPort` to `mcpPort` - represents MCP server port

This change addresses user confusion about port semantics as discussed
in issue #1452. The new names clearly indicate which port corresponds
to which component:
- proxyPort: Port exposed by the proxy runner in Kubernetes
- mcpPort: Port that the MCP server listens on internally

Signed-off-by: ChrisJBurns <[email protected]>
@ChrisJBurns ChrisJBurns force-pushed the claude/issue-1452-20250827-1021 branch from 31cc2b7 to aaa2be4 Compare October 24, 2025 20:27
@ChrisJBurns
Copy link
Collaborator Author

@claude can you rebase this with main please

@claude
Copy link
Contributor

claude bot commented Oct 25, 2025

Claude finished @ChrisJBurns's task —— View job


Rebasing with main

  • Check current branch status
  • Identify commits behind/ahead
  • Explain the situation and provide recommendation

Analysis

I've investigated the current state and discovered an important situation:

This PR's work has already been merged to main in PR #2330!

The commit 3492223 ("adds new port values for MCPServer CRD (#2330)") on origin/main contains the same port renaming work from this PR. However, there are some differences:

Comparison:

  • This PR branch (claude/issue-1452-20250827-1021): Contains the port renaming changes
  • Origin/main: Contains the same port renaming changes PLUS additional updates to pkg/registry/data/registry.json (109 lines added)

The main difference is that origin/main has a more recent version of the registry data that this branch doesn't have.

Recommendation

Since this PR's core functionality has already been merged via PR #2330, I recommend:

Option 1 (Recommended): Close this PR

Option 2: Update this PR to only include any missing differences

Why I cannot rebase automatically:

I don't have permission to perform git rebase operations. These are branch operations that require explicit user approval. Even if you wanted to proceed with a rebase, you would need to:

git fetch origin
git rebase origin/main
# Resolve any conflicts
git push origin claude/issue-1452-20250827-1021 --force-with-lease

However, given that the work is already merged, a rebase would likely result in conflicts or no-op changes.

What would you like to do? Close this PR since the work is already in main, or is there something specific from this branch that's not in #2330 that you want to preserve?

--- | Branch

@ChrisJBurns ChrisJBurns enabled auto-merge (squash) October 25, 2025 16:33
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

I pointed out some cases of fields that look to me like they should be update to the compat calls. Apart from them, I think we should update the examples to use the new fields, but maybe as another PR after the release so we don't have a race where the examples in git and linked from docs are using the new fields but the new fields are not released yet?

There are also examples of the CRDs using the old schema in the architecture guide which might be updated right away.

@ChrisJBurns
Copy link
Collaborator Author

@jhrozek Can't believe I missed those, it seems my CMD + SHIFT + F failed me! Repushed.

Signed-off-by: ChrisJBurns <[email protected]>
Signed-off-by: ChrisJBurns <[email protected]>
@ChrisJBurns ChrisJBurns merged commit eb4231a into main Oct 27, 2025
28 of 29 checks passed
@ChrisJBurns ChrisJBurns deleted the claude/issue-1452-20250827-1021 branch October 27, 2025 17:33
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.

5 participants