Backport #458 to development: fix publish crash with embedded db credentials - #467
Merged
Conversation
* Require --db-server for embedded credentials/oauth workbook publishing, fix connections payload * Translations for publish.options.db-server message * Simplify changes * Adjust tests
PR #458 fixed the two publish crashes when using --db-username / --db-password / --save-db-password, but callers who omit --db-server still hit a raw ValueError from tableauserverclient's _add_connections_element. Fail fast in run_command with a clear, localizable message before we build the ConnectionItem so users get an actionable error instead of an internal stack trace. Update the existing no-db-server test to assert the exit path and add the parallel coverage for the --oauth-username branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extend the online publish suite with: - A positive assertion for the happy path: test_wb_publish_embedded now passes --db-server matching the connection host baked into EmbeddedCredentials.twb (see-internal-slack). Without a matching server_address, tableauserverclient silently drops the embedded credentials at publish time, so covering the match is what actually proves the fix. - A negative assertion: test_wb_publish_embedded_missing_db_server_fails runs publish with --db-username but no --db-server and expects a non-zero exit. The check now fires in run_command before any network work, so hardcoded placeholder creds are sufficient. Threads a db_server keyword through the _publish_creds_args helper and records the workbook's connection host as a class constant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Scope the guard to workbook publishes only. Datasource publishes go through _add_credentials_element on the TSC side, which never requires server_address, so the previous unconditional check was a regression for `tabcmd publish live_mysql.tds --db-username ...` and similar. The guard now checks the target filename extension early (before auth, for fast failure) and repeats per-file inside the workbook branch of the loop for the folder-publish case. Rename the `credentials` parameter of `publish_workbook_file` to `connection` — it holds a single ConnectionItem, not a list. Tests: - Add test_publish_with_oauth_creds covering the oauth branch's server_address assignment (previously untested). - Add test_publish_datasource_with_db_username_no_db_server verifying datasource publishes are not blocked when --db-server is omitted. - Strengthen the e2e negative test to run tabcmd via subprocess.run, capture stdout/stderr, and assert on the localized guard message (or the raw key when .mo has not been regenerated). Any earlier unrelated failure — bad auth, missing asset, session expiry — now fails the test rather than passing it. - Trim the misleading class-constant comment on TWB_FILE_EMBEDDED_ CONNECTION_SERVER; reword the "before we touch the database" comment to reference the guard's contract instead of a specific location. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This reverts commit cfe52c1.
This reverts commit 9581329.
This reverts commit d59ee65.
jacalata
changed the title
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The publish path still allows --db-username/--oauth-username with no --db-server (risking the original runtime failure) and the new locale keys appear not to be propagated into the checked-in .mo catalogs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the publish command to better support publishing workbooks with embedded database credentials by adding a --db-server CLI option and passing workbook connection credentials in the shape expected by tableauserverclient.
Changes:
- Add
--db-serverpublish CLI argument and wire it into the workbookConnectionItem.server_address. - Fix workbook publish call to pass
connectionsas a list ([ConnectionItem]) instead of a singleConnectionItem. - Add/adjust unit tests and add localized help text for the new option across supported locales.
File summaries
| File | Description |
|---|---|
tabcmd/commands/datasources_and_workbooks/publish_command.py |
Sets server_address from --db-server and wraps publish connections in a list. |
tabcmd/execution/global_options.py |
Adds the --db-server argument to the publish command parser. |
tests/commands/test_publish_command.py |
Extends publish credential tests to assert connections list handling and server_address propagation. |
tabcmd/locales/en/tabcmd_messages_en.properties |
Adds help text for publish.options.db-server. |
tabcmd/locales/de/tabcmd_messages_de.properties |
Adds help text for publish.options.db-server. |
tabcmd/locales/es/tabcmd_messages_es.properties |
Adds help text for publish.options.db-server. |
tabcmd/locales/fr/tabcmd_messages_fr.properties |
Adds help text for publish.options.db-server. |
tabcmd/locales/ga/tabcmd_messages_ga.properties |
Adds help text for publish.options.db-server. |
tabcmd/locales/it/tabcmd_messages_it.properties |
Adds help text for publish.options.db-server. |
tabcmd/locales/ja/tabcmd_messages_ja.properties |
Adds help text for publish.options.db-server. |
tabcmd/locales/ko/tabcmd_messages_ko.properties |
Adds help text for publish.options.db-server. |
tabcmd/locales/pt/tabcmd_messages_pt.properties |
Adds help text for publish.options.db-server (with a Portuguese grammar fix suggested in review). |
tabcmd/locales/sv/tabcmd_messages_sv.properties |
Adds help text for publish.options.db-server. |
tabcmd/locales/zh/tabcmd_messages_zh.properties |
Adds help text for publish.options.db-server. |
Review details
- Files reviewed: 14/24 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
69
to
73
| if args.db_username: | ||
| creds = TSC.models.ConnectionCredentials(args.db_username, args.db_password, embed=args.save_db_password) | ||
| workbook_connections = TSC.ConnectionItem() | ||
| workbook_connections.server_address = args.db_server | ||
| workbook_connections.connection_credentials = creds |
Comment on lines
+89
to
+93
| mock_args.db_username = "username" | ||
| mock_args.db_password = "oauth_u" | ||
| mock_args.db_server = None | ||
| mock_args.save_db_password = True | ||
| mock_args.oauth_username = None |
| publish.errors.server_resource_not_found=The resource you specified does not exist, or you do not have permission to see it. Check your project name and permissions | ||
| publish.options.append=Append extract file to existing data source | ||
| publish.options.db-password=Database password for all data sources | ||
| publish.options.db-server=Server address of the database to associate with the embedded connection credentials. Required when using --db-username or --oauth-username to publish a workbook. |
| publish.errors.server_resource_not_found=O recurso especificado não existe ou você não tem permissão para vê-lo. Verifique o nome e as permissões do projeto | ||
| publish.options.append=Anexe o arquivo de extração à fonte de dados existente | ||
| publish.options.db-password=Senha do banco de dados para todas as fontes de dados | ||
| publish.options.db-server=Endereço do servidor do banco de dado associado as credenciais. Obrigatório ao usar --db-username ou --oauth-username. |
This was referenced Sep 10, 2026
yyk16293
approved these changes
Sep 10, 2026
Contributor
Author
|
Note on the Copilot review above (2026-09-10 20:12 UTC): This PR has been intentionally rescoped to a pure cherry-pick of #458 to unblock the crash fix, and the four Copilot findings all land in stacked follow-up PRs:
Stack: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Direct backport of #458 (Mauro Fujihara's fix, already merged to
main) to thedevelopmentbranch. Preserves Mauro's authorship onbbfaf4c.What this PR does
Backports Mauro's two-line fix for the publish-with-embedded-credentials crash:
ConnectionItemin a list on theserver.workbooks.publish(..., connections=...)call (fixesTypeError: object of type 'ConnectionItem' has no len()).--db-serverCLI flag so users can populateConnectionItem.server_address(fixes the downstreamValueError: Connection must have a server address).Scope
Intentionally scoped to just #458's cherry-pick — no follow-up validation, no follow-up tests, no i18n regeneration. Those ship as separate PRs stacked on top of this one:
tabcmd/locales/en/LC_MESSAGES/tabcmd.mo— needed because the cherry-pick updated.propertiesbut the English.mobinary wasn't refreshed, so English users would see raw msgid keys.--db-serverguard, e2e coverage, and review polish (helper extraction, silent-drop caveat in help text).The four revert commits at the tip cancel out earlier follow-up work — net diff is exactly what #458 landed on
main.Test plan
pytest tests/commands/test_publish_command.py— 10 pass locally.c:/dev/notes/work/tabcmd/Report - PR 467 review.md.🤖 Generated with Claude Code