Skip to content

Conversation

@pyramation
Copy link
Contributor

core(migrate): ATTEMPT2 — single-source dep resolution; trust empty arrays; qualify+dedupe; fix tag-based test expectation

Summary

Fixes a critical bug where tags were leaking into launchql_migrate.deploy() database procedure, causing "Missing required changes" errors. The core issue was that when dependency resolution returned empty arrays, the code incorrectly fell back to raw plan dependencies (which could contain unresolved tags like package:@tag) instead of trusting the resolver's output.

Key Changes:

  • Fixed tag leakage: Changed fallback logic to trust empty dependency arrays from resolver (resolvedFromDeps !== undefined instead of checking length)
  • Changed default dependency source: Now defaults to 'plan' instead of 'sql' (options.usePlan === false ? 'sql' : 'plan')
  • Added dependency qualification: Internal deps now get package prefix (dep.includes(':') ? dep : ${package}:${dep})
  • Added deduplication: Uses Array.from(new Set(...)) to avoid DB constraint violations
  • Updated test expectations: Fixed tag-based migration test to expect correct dependency (create_table not create_another_table)
  • Refactored module listing: Moved listModules logic into LaunchQLPackage class with collision handling

Review & Testing Checklist for Human

⚠️ HIGH RISK ITEMS (3):

  • Verify default behavior change doesn't break existing functionality: The dependency source default changed from 'sql' to 'plan'. Test existing deployments that don't explicitly set usePlan to ensure they still work correctly
  • Test dependency qualification/deduplication logic thoroughly: The new logic in qualifiedDeps could have edge cases. Test scenarios with mixed internal/external deps, duplicate deps, and various package naming patterns
  • Confirm test expectation changes are actually correct: The tag-based migration test expectation was updated from create_another_table to create_table. Manually verify this matches the actual fixture dependencies in /home/ubuntu/repos/launchql/__fixtures__/sqitch/simple-w-tags/

Recommended End-to-End Test Plan:

  1. Deploy the original failing scenario (cli-deploy-stage-unique-names.test.ts) to confirm fix
  2. Test both plan-based and sql-based dependency resolution with various tag scenarios
  3. Test cross-project dependencies with the new qualification logic
  4. Verify existing deployments still work with the new default source behavior

Notes

devin-ai-integration bot and others added 5 commits August 27, 2025 03:30
…lify+dedupe deps; doc ATTEMPT2.md

Co-Authored-By: Dan Lynch <[email protected]>
…res (my-third depends on my-second:@v2.0.0 -> create_table)

Co-Authored-By: Dan Lynch <[email protected]>
@devin-ai-integration
Copy link
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

pyramation added a commit that referenced this pull request Sep 3, 2025
pyramation added a commit that referenced this pull request Sep 3, 2025
@pyramation pyramation closed this Sep 3, 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.

2 participants