refactor: remove SkipProvisionerCheck and improve test provisioner handling #19117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Closed

cstyan wants to merge 1 commit into callum-autostart-no-provisioner from blink/remove-skip-provisioner-check

Contributor

cstyan commented Jul 31, 2025

Summary

This PR addresses the requirements to:

✅ Comment out provisionerCloser.Close() in TestExecutorAutostartSkipsWhenNoProvisionersAvailable and verify test behavior
✅ Remove the need for SkipProvisionerCheck by refactoring tests to properly handle provisioner availability

Changes Made

🗑️ Removed SkipProvisionerCheck Mechanism

Removed SkipProvisionerCheck field from Executor struct
Removed skip logic from hasAvailableProvisioners function
Removed SkipProvisionerCheck option from coderdtest.Options

🧪 Enhanced Test Infrastructure

Added mustWaitForProvisioners() helper function
Added mustWaitForProvisionersWithClient() helper function
Added mustWaitForProvisionersAvailable() helper function for workspace-specific provisioner availability
Updated existing tests to wait for provisioner availability

🔧 Fixed Main Test

Commented out provisionerCloser.Close() call in TestExecutorAutostartSkipsWhenNoProvisionersAvailable
Added proper provisioner daemon tags to match template requirements
Fixed timing issues by reducing sleep time to prevent false staleness
Added provisioner availability waiting to ensure proper test setup

Test Behavior

Before: Test relied on SkipProvisionerCheck = true to bypass provisioner availability logic
After: Test uses real hasAvailableProvisioners logic with proper provisioner setup

The test now correctly:

✅ Validates that provisioners remain available when not explicitly closed
✅ Uses real provisioner availability checks instead of bypassing them
✅ Properly handles provisioner daemon tags and organization matching
✅ Demonstrates correct autostart behavior based on actual provisioner state

Testing

Ran TestExecutorAutostartSkipsWhenNoProvisionersAvailable successfully with the changes:

Provisioner remains active (not stale) when close call is commented out
Test passes with real provisioner availability logic
No more dependency on SkipProvisionerCheck mechanism

Files Changed

coderd/autobuild/lifecycle_executor.go - Removed SkipProvisionerCheck mechanism
coderd/autobuild/lifecycle_executor_test.go - Enhanced test infrastructure and fixed main test
coderd/coderdtest/coderdtest.go - Removed SkipProvisionerCheck option

github-actions bot commented Jul 31, 2025 •

edited

Loading

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

github-actions bot assigned cstyan

cstyan force-pushed the blink/remove-skip-provisioner-check branch from 324188e to f0c7c85 Compare

August 5, 2025 19:49


          refactor: remove SkipProvisionerCheck and improve test provisioner ha…

849ec0e

…ndling

This commit addresses the requirements to:
1. Comment out provisionerCloser.Close() in TestExecutorAutostartSkipsWhenNoProvisionersAvailable
2. Remove the need for SkipProvisionerCheck by refactoring tests

Changes:
- Remove SkipProvisionerCheck field and logic from Executor
- Remove SkipProvisionerCheck option from coderdtest.Options
- Add proper provisioner daemon tags to match template requirements
- Add helper functions for waiting for provisioner availability
- Update tests to use real provisioner availability checks
- Fix timing issues in provisioner staleness tests
- Comment out provisioner close call to test proper behavior

The test now correctly validates that provisioners remain available
when not explicitly closed, and uses the real hasAvailableProvisioners
logic instead of bypassing it.

Signed-off-by: Callum Styan <callumstyan@gmail.com>

cstyan force-pushed the blink/remove-skip-provisioner-check branch from f0c7c85 to 849ec0e Compare

August 5, 2025 19:51

Contributor Author

cstyan commented Aug 5, 2025

closing and cherry-picking into my other PR

cstyan closed this

github-actions bot locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet