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

acc: Added acceptance test for CLI commands inside bundle with and without profile flag #2270

Merged
merged 11 commits into from
Feb 5, 2025

Conversation

andrewnester
Copy link
Contributor

Changes

This encodes existing behaviour in CLI as reported here: #1358

acceptance/auth/bundle_and_profile/script.prepare Outdated Show resolved Hide resolved
acceptance/auth/bundle_and_profile/script.cleanup Outdated Show resolved Hide resolved
acceptance/auth/bundle_and_profile/script.prepare Outdated Show resolved Hide resolved
acceptance/auth/bundle_and_profile/test.toml Outdated Show resolved Hide resolved
acceptance/auth/bundle_and_profile/.databrickscfg Outdated Show resolved Hide resolved
acceptance/auth/bundle_and_profile/script Outdated Show resolved Hide resolved
@@ -0,0 +1 @@
Badness = "When -p flag is used inside the bundle folder for any CLI commands, CLI use bundle host anyway instead of profile one"
Copy link
Contributor

Choose a reason for hiding this comment

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

How do I know from the output which host is used? The commands only output the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using correct host meaning current-user me returns a expected user. There are no CLI commands (API) which returns host being used, so we can't really use server stubs unless we deviate from API responses.
We could use describe auth describe command to see the host but this would deviate a bit from idea of this test: "I want my autogenerated CLI commands to make calls against correct host depending on flags I pass"

Copy link
Contributor

Choose a reason for hiding this comment

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

One idea on testing different hosts that comes to mind (up to you if you think it's worth it):

  • Set up 2 servers in the test runner, $DATABRICKS_HOST and $DATABRICKS_OTHER_HOST
  • Update RequestLog added by @shreyas-goenka feature to include host that is being used.

acceptance/auth/bundle_and_profile/output.txt Outdated Show resolved Hide resolved
targets:
dev:
default: true
workspace:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We don't have to override the same host here since it's the default target

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, but I just made it explicit

"$USERNAME"

=== Inside the bundle, target and matching profile
>>> errcode $CLI current-user me -t dev -p DEFAULT
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This doesn't provide any extra coverage since -t dev is implicit in the configuration. Same below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, because it test what happens if we provide both -t and -p with matching hosts.
because we could make a change making -t and -p exclusive and this test would break

@andrewnester andrewnester added this pull request to the merge queue Feb 3, 2025
@denik denik removed this pull request from the merge queue due to a manual request Feb 3, 2025
Copy link
Contributor

@denik denik left a comment

Choose a reason for hiding this comment

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

(see comments)

@andrewnester andrewnester added this pull request to the merge queue Feb 5, 2025
Merged via the queue into main with commit 5c90752 Feb 5, 2025
9 checks passed
@andrewnester andrewnester deleted the acc/cli-bundle-auth branch February 5, 2025 11:59
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.

4 participants