-
Notifications
You must be signed in to change notification settings - Fork 119
Fix oss-source pipeline errors when using the Databricks CLI #3889
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.
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
Conversation
38 failing tests:
|
| FastValidate: true, | ||
| Build: true, | ||
| Deploy: true, | ||
| IsPipelinesCLI: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This scopes it down to deploy, but it seems this is something that would be useful in plan & validate as well, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the previous implementation, the error only triggered on deploy. I'll leave it updo the pipelines team if they want this during dry run.
Note pipeliens CLI does not have plan or validate, only a dryrun command.
| return b, isDirectEngine, nil | ||
| } | ||
|
|
||
| func rejectDefinitions(ctx context.Context, b *bundle.Bundle) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit odd to have this here, this package orchestrates existing phases rather than having actual validation logic.
What is we keep it in initialize.go() and then detect pipelines exe via argv[0]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Detecting via argv[0] feels hacky. I don't mind recording IsPipelineCLI in the bundle tree maybe and then record this in initialize.
IMO the current implementation is fine but I'm open to any alternate suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One advantage is it's obvious which code is for pipelines only. You don't have to dive into the guts (the initialize phase here) to see whether some code is for pipelines CLI only or not.
Changes
Removes validation that incorrectly rejected bundles using the
definitionsfield indatabricks.yml.Why
PR #3797 inadvertently applied validation to reject OSS Spark Declarative Pipelines to all Databricks CLI bundles. This error was only meant for the pipelines CLI. Customers commonly use the
definitionsfield for YAML anchors (e.g., to reuse cluster configurations), and this validation blocked their deployments with the error:The validation has been removed entirely. The Pipelines CLI is not yet in active use, and the pipelines team can add a better context-aware check later that only applies when running as the Pipelines CLI.
Tests
acceptance/bundle/validate/definitions_yaml_anchors/verifies bundles withdefinitionsfield validate successfullyacceptance/pipelines/deploy/oss-spark-error/ensures the error continues to work for the pipelines CLI.