Skip to content

Conversation

@yrobla
Copy link
Contributor

@yrobla yrobla commented Sep 8, 2025

modify the operator to use it instead of the command line arguments use checksums to force redeployment of resources
update tests to reflect the new changes

Closes: #1638

@yrobla yrobla requested a review from Copilot September 8, 2025 08:41

This comment was marked as outdated.

@yrobla yrobla force-pushed the issue-1638-use-configmap branch from 9fefd3b to 80db195 Compare September 8, 2025 10:58
@codecov
Copy link

codecov bot commented Sep 8, 2025

Codecov Report

❌ Patch coverage is 68.00000% with 40 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.91%. Comparing base (0392dcf) to head (658f140).
⚠️ Report is 39 commits behind head on main.

Files with missing lines Patch % Lines
pkg/container/kubernetes/client.go 59.21% 24 Missing and 7 partials ⚠️
pkg/runner/config_builder.go 30.76% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1766      +/-   ##
==========================================
+ Coverage   41.78%   41.91%   +0.13%     
==========================================
  Files         184      185       +1     
  Lines       21533    21627      +94     
==========================================
+ Hits         8997     9066      +69     
- Misses      11841    11863      +22     
- Partials      695      698       +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.

@coveralls
Copy link
Collaborator

coveralls commented Sep 8, 2025

Coverage Status

coverage: 38.792% (+0.3%) from 38.516%
when pulling aed0019 on issue-1638-use-configmap
into cf5e7ea on main.

@yrobla yrobla force-pushed the issue-1638-use-configmap branch 3 times, most recently from 9706236 to 89edb65 Compare September 8, 2025 11:35
@yrobla yrobla requested a review from Copilot September 8, 2025 11:42

This comment was marked as outdated.

@yrobla yrobla force-pushed the issue-1638-use-configmap branch from 89edb65 to 903cf6c Compare September 8, 2025 12:08
@yrobla yrobla requested review from ChrisJBurns and JAORMX September 8, 2025 13:00
@yrobla yrobla force-pushed the issue-1638-use-configmap branch 4 times, most recently from dacc28d to 0d5a2c8 Compare September 8, 2025 13:26
@yrobla yrobla requested a review from Copilot September 8, 2025 13:26

This comment was marked as outdated.

@yrobla yrobla requested a review from Copilot September 8, 2025 13:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR modifies the proxy runner to use a ConfigMap approach for configuration instead of command line arguments, implementing checksums for force redeployment of resources and updating tests to reflect the new changes.

Key changes:

  • Replace individual CLI arguments with --from-configmap flag in proxy runner
  • Add ConfigMap checksum support for tracking configuration changes and triggering redeployments
  • Update operator to generate RunConfig ConfigMaps and use ConfigMap-based deployment

Reviewed Changes

Copilot reviewed 18 out of 20 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/e2e/proxy_runner_configmap_test.go New E2E tests for ConfigMap-based proxy runner functionality
test/e2e/chainsaw/operator/single-tenancy/test-scenarios/sse/test-configmap-approach.yaml Integration test validating ConfigMap approach in SSE transport scenarios
test/e2e/chainsaw/operator/*/test-scenarios/common/proxyrunner-role.yaml Added ConfigMap read permissions for proxy runner RBAC
pkg/util/configmap.go New utility function for computing ConfigMap checksums
pkg/util/configmap_test.go Unit tests for ConfigMap checksum computation
pkg/runner/config.go Added ConfigMapChecksum field to RunConfig structure
pkg/runner/config_builder.go Default ProxyMode handling for stdio transport
pkg/runner/runner.go Integration of ConfigMap checksum with Kubernetes deployer
pkg/container/kubernetes/client.go Refactored deployment logic and added ConfigMap checksum annotation support
docs/server/swagger.yaml Updated API documentation for new ConfigMapChecksum field
cmd/thv-proxyrunner/app/run.go Added --from-configmap flag and ConfigMap loading functionality
cmd/thv-proxyrunner/app/run_test.go Unit tests for new ConfigMap-based configuration
cmd/thv-operator/controllers/mcpserver_controller.go Modified operator to use ConfigMap-based deployment approach
cmd/thv-operator/controllers/mcpserver_runconfig.go Moved checksum computation to shared utility
cmd/thv-operator/controllers/mcpserver_runconfig_test.go Updated tests to use shared checksum utility
cmd/thv-operator/controllers/mcpserver_pod_template_test.go Updated tests for ConfigMap-based pod template configuration
cmd/thv-operator/controllers/mcpserver_resource_overrides_test.go Removed duplicate/unused test function

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@yrobla yrobla force-pushed the issue-1638-use-configmap branch 2 times, most recently from 2e64f31 to aed0019 Compare September 8, 2025 14:31
@yrobla
Copy link
Contributor Author

yrobla commented Sep 8, 2025

i will split in smaller prs as @ChrisJBurns requested. I know this grows and grows!

@yrobla yrobla marked this pull request as draft September 9, 2025 08:44
modify the operator to use it instead of the command line arguments
use checksums to force redeployment of resources
update tests to reflect the new changes

Closes: #1638
@yrobla yrobla force-pushed the issue-1638-use-configmap branch from aed0019 to 658f140 Compare September 9, 2025 13:31
@yrobla yrobla closed this Sep 12, 2025
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.

Stop using local files for persistence

5 participants