Skip to content

Conversation

@driftx
Copy link

@driftx driftx commented Oct 1, 2025

What is the issue

GuardrailsOptions never calls validate() on the configuration so exceptions aren't thrown, which breaks the GuardrailsConfigUpdaterTest

What does this PR fix and why was it fixed

Calls validate(), fixes #15537

@github-actions
Copy link

github-actions bot commented Oct 1, 2025

Checklist before you submit for review

  • This PR adheres to the Definition of Done
  • Make sure there is a PR in the CNDB project updating the Converged Cassandra version
  • Use NoSpamLogger for log lines that may appear frequently in the logs
  • Verify test results on Butler
  • Test coverage for new/modified code is > 80%
  • Proper code formatting
  • Proper title for each commit staring with the project-issue number, like CNDB-1234
  • Each commit has a meaningful description
  • Each commit is not very long and contains related changes
  • Renames, moves and reformatting are in distinct commits
  • All new files should contain the DataStax copyright header instead of the Apache License one

@driftx driftx requested a review from djatnieks October 1, 2025 12:22
@cassci-bot
Copy link

❌ Build ds-cassandra-pr-gate/PR-2032 rejected by Butler


1 regressions found
See build details here


Found 1 new test failures

Test Explanation Runs Upstream
o.a.c.index.sai.cql.datamodels.QueryWriteLifecycleWithCompositePartitionKeyTest.terminated successfully () <span title="The test is NOT RUN on upstream but has a SINGLE FAILURE on feature:
• Feature branch: a SINGLE FAILURE in 1 builds (1 to 1)
• Upstream branch: MISSING in the last run in 9 builds (147 to 156)">NEW 🔴 0 / 9

Found 5 known test failures

Copy link

@djatnieks djatnieks left a comment

Choose a reason for hiding this comment

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

I think the real root problem here is that when I originally worked on guardrails in CC and CNDB I didn't have a validate method at all in CC - this was recently corrected - and then on the CNDB side in GuardrailsConfigUpdaterTest I left out the call to validate in GuardrailsConfigUpdater.updateField (since it didn't exist).

So, I think we should fix CNDB GuardrailsConfigUpdater.updateField to now call validate, e.g.

        try
        {
            // Validate the new guardrail config
            new GuardrailsOptions(config).validate();
        }

I had a comment saying "validate" but the code didn't call it like the CNDB main branch code does (since it didn't exist yet in CC 5.0).

@driftx
Copy link
Author

driftx commented Oct 1, 2025

That makes sense, closing this in favor of doing that.

@driftx driftx closed this Oct 1, 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.

3 participants