Fix publish failures when using embedded db credentials - #458
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Attempts to fix credentialed workbook publishing for TSC 0.41.
Changes:
- Adds
--db-serverand assigns it to workbook connections. - Passes workbook connections as a list and adds tests.
- Adds localized help and recompiles catalogs.
Reviewed changes
Copilot reviewed 14 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tabcmd/commands/datasources_and_workbooks/publish_command.py |
Builds and passes workbook connections. |
tabcmd/execution/global_options.py |
Adds --db-server. |
tests/commands/test_publish_command.py |
Tests connection payloads. |
tabcmd/locales/{de,en,es,fr,ga,it,ja,ko,pt,sv,zh}/tabcmd_messages_*.properties |
Adds localized option help. |
tabcmd/locales/{de,en,es,fr,ga,it,ja,ko,pt,sv,zh}/LC_MESSAGES/tabcmd.mo |
Updates compiled catalogs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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 |
Contributor
There was a problem hiding this comment.
This comment is correct. You need to update your test to fail when no server is given - if you also want to add the check in run_command that'd be great, but it's not necessary before this gets checked in.
| publish.options.append=Anexe o arquivo de extração à fonte de dados existente | ||
| publish.options.async=Publicar de forma assíncrona | ||
| 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. |
Contributor
There was a problem hiding this comment.
This is fine unless you want to make the edits. I will do the package regeneration with new messages.
Contributor
|
Sorry for the slow response - one change needed to the test mentioned, otherwise it looks great. |
jacalata
approved these changes
Sep 8, 2026
jacalata
left a comment
jacalata
left a comment
Contributor
There was a problem hiding this comment.
Validated end-to-end against 10ax; publish with embedded db creds succeeds when server_address matches. Landing this and will push a followup with the missing-server test.
This was referenced Sep 9, 2026
jacalata
added a commit
that referenced
this pull request
Sep 10, 2026
…entials (#467) * Fix publish failures when using embedded db credentials (#458) * Require --db-server for embedded credentials/oauth workbook publishing, fix connections payload * Translations for publish.options.db-server message * Simplify changes * Adjust tests * Require --db-server when publishing with embedded credentials 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> * Add e2e coverage for --db-server requirement 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> * Address code-review findings on the --db-server guard 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> * Revert "Address code-review findings on the --db-server guard" This reverts commit cfe52c1. * Revert "Add e2e coverage for --db-server requirement" This reverts commit 9581329. * Revert "Require --db-server when publishing with embedded credentials" This reverts commit d59ee65. --------- Co-authored-by: Mauro Fujihara <fujihara@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
--db-username/--db-password/--save-db-passwordor--oauth-usernamealways failed withValueError: Connection must have a server address, because theConnectionItembuilt for the workbook'sconnections=payload never setserver_address.tableauserverclient0.41 now hard-requires it.publish_workbook_file()passed the bareConnectionIteminstead of wrapping it in a list, causingTypeError: object of type 'ConnectionItem' has no len().--db-serverCLI flag so users can supply the database server address to associate with the embedded connection credentials. When omitted,server_addressis left unset (None), matching the flag's optional nature.publish_workbook_file()to passconnections=[credentials](a list) instead of the bareConnectionItem, matching whattableauserverclientexpects.publish.options.db-server) across all supported locales (de, en, es, fr, ga, it, ja, ko, pt, sv, zh), including recompiled.mocatalogs.Test plan
python -m pytest tests/ -v— 318 passed, 2 skipped (pre-existing, unrelated), 0 failedpython -m pytest tests/commands/test_publish_command.py -v— 10/10 passing, including coverage forserver_addressbeing set from--db-serverand for theNonecase, and for the wrapped-listconnectionspayloadtabcmd publish --helprenders the new--db-serverflag and localized help text (spot-checkedenandde/ptviaLANGoverride)server.workbooks.publishcall)