-
Notifications
You must be signed in to change notification settings - Fork 101
New hidden install-pipelines-cli
command that creates pipelines
symlink to databricks
#3009
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
…atabricks CLI with tests
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.
Left few comments
acceptance/acceptance_test.go
Outdated
@@ -150,7 +150,8 @@ func testAccept(t *testing.T, inprocessMode bool, singleTest string) int { | |||
repls.SetPath(execPath, "[CLI]") | |||
|
|||
// Make helper scripts available | |||
t.Setenv("PATH", fmt.Sprintf("%s%c%s", filepath.Join(cwd, "bin"), os.PathListSeparator, os.Getenv("PATH"))) | |||
pathDirs := []string{filepath.Join(cwd, "bin"), buildDir} | |||
t.Setenv("PATH", fmt.Sprintf("%s%c%s", strings.Join(pathDirs, string(os.PathListSeparator)), os.PathListSeparator, os.Getenv("PATH"))) |
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 adds buildDir to PATH? What for?
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.
If the goal is to expose dlt command to test then we should
- modify BuildCLI to link databricks to dlt
- export $DLT env var pointing to built dlt (+ replacement)
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.
The intention here was probably so dlt can be found on the build path when it's built, but we should probably export the $DLT env var the same way we do it for CLI and other envs.
qq: maybe I misunderstood, wdym by "+ replacement" here?
@denik / @kanterov high level question here: we should be definitely adding acceptance tests, but in general does it hurt to have unit tests as well? A quick look through the repo and it feels like some files have more unit tests while others don't. For this command specifically, I think it's helpful to test the edge cases for which PATH might not contain databricks, target local isn't writable etc... WDYT? |
Depends on code being test; if it's a pure function / class, makes sense to cover edge cases with unit tests. If it involves reading env vars, command line arguments then acceptance tests typically provide more realistic setup. |
8711e41
to
ca3d09b
Compare
d1f1df0
to
d168e96
Compare
085a428
to
9d53ff8
Compare
ac4799e
to
45c7df1
Compare
45c7df1
to
12b1e64
Compare
acceptance/dlt/install-dlt/script
Outdated
|
||
title "Executable not called as databricks" | ||
cp $CLI notdatabricks | ||
dlt=$(trace errcode ./notdatabricks install-dlt -o json | jq -r .symlink_path) |
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 also not clear here what's being asserted.
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.
In the case that the user has databricks aliased, want to make sure the install-dlt command still works.
554264e
to
515956f
Compare
515956f
to
1da3ff0
Compare
main.go
Outdated
// binary to be used for both DLT and databricks CLI commands. | ||
func getCommand(ctx context.Context) *cobra.Command { | ||
invokedAs := filepath.Base(os.Args[0]) | ||
if invokedAs == "dlt" || (runtime.GOOS == "windows" && invokedAs == "dlt.exe") { |
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.
nit: you can replace it with strings.StartsWith
for invokedAs
as nit
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.
Approved to unblock but first symlink needs to be renamed from dlt to pipelines
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.
Please update the PR title and summary to reflect the new state of the PR.
dlt
symlink to databricks
pipelines
symlink to databricks
pipelines
symlink to databricks
install-pipelines-cli
command that creates pipelines
symlink to databricks
0fd2c95
to
9156ad5
Compare
An authorized user can trigger integration tests manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
Changes
pipelines.go
(packagepipelines
)install_pipelines_cli.go
(packagepipelines
)databricks
, implements a hiddeninstall-pipelines-cli
command that creates a symlink namedpipelines
pointing to the realdatabricks
binary.pipelines/install-pipelines-cli
acceptance testmain.go
returns a root depending on if the binary is invoked aspipelines
/pipelines.exe
ordatabricks
Why
pipelines
as a standalone CLI, which internally points to the maindatabricks
binary. This is a step towards a dedicated Pipelines CLI experience, while minimizing code duplication.Tests
install-pipelines-cli
in pipelines to check if success messagepipelines successfully installed
is printedCreates temporary directory and verifies successful symlink creation.
Validates CLI output and stub command functionality.
Confirms graceful handling when symlink already exists.
Ensures installation fails when regular file exists at target path.
Verifies installation works when databricks binary is renamed/called via alias.