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

Try to resolve a profile if only the host is specified #287

Merged
merged 3 commits into from
Mar 29, 2023

Conversation

pietern
Copy link
Contributor

@pietern pietern commented Mar 29, 2023

Changes

This improves out of the box usability where a user who already configured a .databrickscfg file will be able to reference the workspace host in their bundle.yml and it will automatically pick up the right profile.

Tests

  • Newly added tests pass.
  • Manual testing confirms intended behavior.

This improves out of the box usability where a user who already
configured a `.databrickscfg` file will be able to reference
the workspace host in their `bundle.yml` and it will automatically
pick up the right profile.
@pietern
Copy link
Contributor Author

pietern commented Mar 29, 2023

Note: the file file.go can be removed once databricks/databricks-sdk-go#349 is merged.

Copy link
Contributor

@shreyas-goenka shreyas-goenka left a comment

Choose a reason for hiding this comment

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

Looks good! just some minor comments

// The function expands ~ to the user's home directory.
func LoadFile(path string) (*File, error) {
if path == "" {
path = "~/.databrickscfg"
Copy link
Contributor

Choose a reason for hiding this comment

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

This not work for windows, can be added as a followup though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this functionality from the SDK where it does work on Windows (tests pass and this is covered).

I added your comment to the PR that changes it though, so it can be explored there (link)

Once the File type is merged in the SDK it can be removed here and depended on directly.

}

// Expand ~ to home directory.
if strings.HasPrefix(path, "~") {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It's possible to create a dir named ~foo. Maybe you would like to use filepath.SplitList ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this to the same comment in the SDK. It could use os.PathSeparator.

pietern and others added 2 commits March 29, 2023 20:24
Co-authored-by: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com>
Co-authored-by: shreyas-goenka <88374338+shreyas-goenka@users.noreply.github.com>
@pietern
Copy link
Contributor Author

pietern commented Mar 29, 2023

@shreyas-goenka Thanks for the review.

Punting the Windows comments to the SDK (see databricks/databricks-sdk-go#349).

@pietern pietern merged commit cfd32c9 into main Mar 29, 2023
@pietern pietern deleted the resolve-profile-from-host branch March 29, 2023 18:44
pietern added a commit that referenced this pull request Mar 29, 2023
## Changes

Auth relied on setting a profile. In this change we enumerate all
configuration properties and export all non-empty ones as a map with
environment variables. We then pass this map to the Terraform execution
wrapper.

This results in Terraform using the bundle's authentication
configuration.

This change is needed to make #287 work.

## Tests

Manually.
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