Skip to content
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

go module github.com/databricks/cli contains invalid paths when importing as dependency #1629

Closed
sgerloff opened this issue Jul 27, 2024 · 1 comment · Fixed by #1671
Closed
Assignees
Labels
Bug Something isn't working CLI CLI related issues

Comments

@sgerloff
Copy link

Describe the issue

Inspired by my feature request, I wanted to explore building the functionality myself using github.com/databricks/cli/libs as a dependency to build on top. In essence, exposing your github.com/databricks/cli/libs as valid go modules enables your customers with special needs and fosters a community that voluntarily owns functionality you do not want to maintain.

Steps to reproduce the behavior

  1. Create dummy go project: go mod init whatever
  2. Add databricks cli as dependency go get github.com/databricks/cli

Expected Behavior

go get github.com/databricks/cli finishes without error

Actual Behavior

go get github.com/databricks/cli finishes with error:

go: downloading github.com/databricks/cli v0.224.1
go: github.com/databricks/cli/libs: create zip: libs/template/testdata/template-in-path/template/{{template `dir_name`}}/{{template `file_name`}}: malformed file path "libs/template/testdata/template-in-path/template/{{template `dir_name`}}/{{template `file_name`}}": invalid char '`'
libs/template/testdata/templated-defaults/template/{{template `dir_name`}}/{{template `file_name`}}: malformed file path "libs/template/testdata/templated-defaults/template/{{template `dir_name`}}/{{template `file_name`}}": invalid char '`'

Note

  • It's okay if you don't want to support this usage
  • This should not be read as an escalation of the previous issue, as I am just curious to play with go and find this is a good excuse.
  • I hope this is a trivial fix where ill-formed test data is excluded from the go modules archive.
@sgerloff sgerloff added the CLI CLI related issues label Jul 27, 2024
pietern added a commit that referenced this issue Aug 12, 2024
Use of backticks prevents using the repository as a dependency.

Fixes #1629.
pietern added a commit that referenced this issue Aug 12, 2024
While investigating #1629, I found that Go doesn't allow characters outside the
set documented at https://pkg.go.dev/golang.org/x/mod/module#CheckFilePath.

To fix this, I changed the relevant test case to create the fixtures it needs
instead of loading it from the testdata directory (in `renderer_test.go`).

Some test cases in `config_test.go` depended on templated paths without needing
to do so. In the process of fixing this, I refactored these tests slightly to
reduce dependencies between them.

This change also adds a test case to ensure that all files in the repository
are allowed to be part of a module (per the earlier `CheckFilePath` function).

Fixes #1629.
@pietern
Copy link
Contributor

pietern commented Aug 12, 2024

Thanks for reporting. I hadn't tried this before but the repository should be usable as a module.

That said, don't expect that all libraries under libs to be stable in terms of public API. We may refactor these as needed.

@pietern pietern self-assigned this Aug 12, 2024
@pietern pietern added the Bug Something isn't working label Aug 12, 2024
github-merge-queue bot pushed a commit that referenced this issue Aug 12, 2024
## Changes

While investigating #1629, I found that Go doesn't allow characters
outside the set documented at
https://pkg.go.dev/golang.org/x/mod/module#CheckFilePath.

To fix this, I changed the relevant test case to create the fixtures it
needs instead of loading it from the `testdata` directory (in
`renderer_test.go`).

Some test cases in `config_test.go` depended on templated paths without
needing to do so. In the process of fixing this, I refactored these
tests slightly to reduce dependencies between them.

This change also adds a test case to ensure that all files in the
repository are allowed to be part of a module (per the earlier
`CheckFilePath` function).

Fixes #1629.

## Tests

I manually confirmed I could import the repository as a Go module.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working CLI CLI related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants