Skip to content

Export sandbox label constant (#216)#317

Open
abkaskari wants to merge 9 commits intokubernetes-sigs:mainfrom
abkaskari:fix/sandbox-pod-labels
Open

Export sandbox label constant (#216)#317
abkaskari wants to merge 9 commits intokubernetes-sigs:mainfrom
abkaskari:fix/sandbox-pod-labels

Conversation

@abkaskari
Copy link

@abkaskari abkaskari commented Feb 14, 2026

This PR exports the sandbox label constant to avoid duplication between
the API and controller packages.

Previously, the sandbox label key was defined inside the controller,
which created a risk of contract drift between the API and its consumers.
This change moves the label definition to api/v1alpha1 and exports it
as SandboxNameHashLabel so it can be reused consistently across the
codebase.

No functional behavior is changed. This is purely a refactoring to
improve maintainability and ensure a single source of truth for the
sandbox label key.

Related to #216.

@k8s-ci-robot
Copy link
Contributor

Hi @abkaskari. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 14, 2026
@netlify
Copy link

netlify bot commented Feb 14, 2026

Deploy Preview for agent-sandbox canceled.

Name Link
🔨 Latest commit c5e619b
🔍 Latest deploy log https://app.netlify.com/projects/agent-sandbox/deploys/69a2bb2a1d346300083bbca3

@vicentefb
Copy link
Member

vicentefb commented Feb 14, 2026

Hello @abkaskari ! do you mind linking the correct issue in the description of the PR, thanks! Perhaps, #216 ?


const (
sandboxLabel = "agents.x-k8s.io/sandbox-name-hash"
//sandboxLabel = "agents.x-k8s.io/sandbox-name-hash"
Copy link
Member

@vicentefb vicentefb Feb 14, 2026

Choose a reason for hiding this comment

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

please remove this line

Copy link
Member

Choose a reason for hiding this comment

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

friendly ping

@vicentefb
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 14, 2026
@abkaskari
Copy link
Author

Hello @abkaskari ! do you mind linking the correct issue in the description of the PR, thanks! Perhaps, #216 ?

#216

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 14, 2026
@abkaskari abkaskari requested a review from vicentefb February 21, 2026 15:20
},
}

for _, tc := range testCases {
Copy link
Member

Choose a reason for hiding this comment

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

There is a massive commented-out block of the old TestReconcilePod function. Please delete this entire commented block.

}

// +kubebuilder:object:root=true

Copy link
Member

Choose a reason for hiding this comment

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

revert this change please

// VolumeClaimTemplates is a list of claims that the sandbox pod is allowed to reference.
// Every claim in this list must have at least one matching access mode with a provisioner volume.
// +optional
// +kubebuilder:validation:Optional
Copy link
Member

Choose a reason for hiding this comment

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

why is this removed ? i believe it should be kept, please revert this change.


// SandboxSpec defines the desired state of Sandbox
type SandboxSpec struct {
// The following markers will use OpenAPI v3 schema to validate the value
Copy link
Member

Choose a reason for hiding this comment

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

revert this change please, it's not related to this PR.

Copy link
Member

@vicentefb vicentefb left a comment

Choose a reason for hiding this comment

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

The actual logic changes—moving the label into api/v1alpha1/sandbox_types.go and exporting it as SandboxNameHashLabel make sense and are exactly the DRY refactoring we needed to prevent contract drift between the controllers. I left some comments that need to be addressed.

I also noticed that the PR description seems to describe a completely different set of changes (e.g., "Ensure existing pods adopt labels", "Update annotations to reflect the latest PodTemplate values", etc.). Could you please update the PR description to accurately reflect the simple constant refactoring you actually did here as well as adding the issue number there ? In addition to this could you also update the PR title, to something Export sandbox label constant, we aren't fixing any pod label synchronization issues here.

Thanks!!

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 23, 2026
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 23, 2026
@abkaskari abkaskari changed the title Fix pod label synchronization issue (#42) Export sandbox label constant (#216) Feb 23, 2026
Copy link
Contributor

@aditya-shantanu aditya-shantanu left a comment

Choose a reason for hiding this comment

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

Thanks for making this change.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: abkaskari, aditya-shantanu
Once this PR has been reviewed and has the lgtm label, please assign soltysh for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@natasha41575
Copy link

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2026
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

const SandboxNameHashLabel = "agents.x-k8s.io/sandbox-name-hash"
Copy link
Member

Choose a reason for hiding this comment

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

Please update extensions/controllers/sandboxclaim_controller.go to reuse this exported label as well

@janetkuo janetkuo self-assigned this Mar 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants