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

Updated some CLI functionality #43

Merged
merged 1 commit into from
Jun 28, 2023

Conversation

JustinGrilli
Copy link
Collaborator

@JustinGrilli JustinGrilli commented May 24, 2023

Changes

  • Converted connection CLI command into a connection argument group
  • Moved connection CLI command logic to datasource and server_operate
    • datasource:
      • Call enforce_connection with the connection args to update the connection in the local datasource
    • server_operate:
      • Provide conn_user and conn_pw when calling --publish datasource to embed creds for the connection when publishing
      • Call embed_connection with connection args to embed connection creds for a datasource online
        • Added validation for embed_connection call
  • Added functionality to server_operate --publish, to open the datasource's URL in the browser once published (Like it would when publishing from Tableau Desktop)
  • Made some updates to the print statements in server_operate
  • Removed some lines that were not used in test_tableau_utilities.py
  • Added --version argument to the CLI, which will reference the version of the installed package
  • Updated readme for connection change
  • Updated sample_settings.yaml and moved to top level folder.
  • Updated a bunch of verbiage in the cli.py help comments.
  • Fixed an issue where, if a YAML was used, but it was not configured correctly, the CLI would fail.
    • Now it will not fail, but if --debugging_logs is provided, it will warn the user.

Notes to reviewer

  • The reason I decided to split the logic from connection between datasource and server_operate is because:
    • After coming back to try and use the CLI, that is where I intuitively expected the functionality to live
    • This should ultimately simplify the CLI
      • There is one less command
      • Server operations are further separated from local operations

Tests

After installing tableau_utilities with these changes:

  • tableau_utilities --version
  • tableau_utilities -d -l online -n Jobs -pn "Official Datasources - Business" -tds datasource --enforce_connection
  • tableau_utilities -d -n Jobs -pn "Official Datasources - Business" --file_path Jobs.tdsx server_operate --publish datasource

@@ -329,6 +336,86 @@ def tableau_authentication(args):
return t


def set_datasource_connection_args(args):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This basically follows the same pattern as the tableau_authentication method above, and is similar to the connection_settings method from the deleted connection.py file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I can test this later. Let's try to do all that merge history stuff and then move forward.

I don't have strong feelings about this being in a separate file or not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good, thanks Jay! :)

@JustinGrilli JustinGrilli force-pushed the updates-and-bug-fixes branch 4 times, most recently from 818b372 to a11e990 Compare May 30, 2023 16:01
@jaybythebay
Copy link
Contributor

@JustinGrilli I'm trying to troubleshoot the weird PR thing. Can you merge this in? That feels like it might make the comparison simpler but perhaps I'm wrong 😮‍💨

@jaybythebay
Copy link
Contributor

@JustinGrilli could you re-run the CI job on this branch? I'm confused as to why this would suddenly fail. If you re-run the CI job here that will let us know if the issue is the other branches OR if it's something in the CI flow from imports/versions etc.

When I google the errors on the other branchs I'm getting package version and pytest version issues. This is an old comment but perhaps it;s relevant pytest-dev/pytest-runner#49 (comment)

@JustinGrilli
Copy link
Collaborator Author

Yeah I will merge this in.

Once I merge it, we should see CI run on the refactor-to-use-restapi PR

@JustinGrilli JustinGrilli merged commit 8d84715 into refactor-to-use-restapi Jun 28, 2023
@JustinGrilli JustinGrilli deleted the updates-and-bug-fixes branch June 28, 2023 15:22
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.

None yet

2 participants