Require --db-server when publishing with embedded credentials by jacalata · Pull Request #473 · tableau/tabcmd

jacalata · 2026-09-18T21:49:31Z

Re-cut of the closed #470 stack — its base branch (jac/regen-mo-post-458) was deleted after #469 merged, orphaning it. Same content, fresh branch off development.

Motivation

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.

Behavior change

Restore the --db-server required-when-embedded guard on the workbook publish path.
Extract _require_db_server_for_workbook(args, filename, logger) helper so the duplicated guard cannot drift between the early pre-auth check and the in-loop per-file check.
Extend the English publish.options.db-server help text to warn about the silent-drop-on-mismatch footgun.
Portuguese grammar fix: banco de dado → banco de dados (plural); associado a credenciais → associado as credenciais (crase).
Regenerate .mo catalogs to pick up the new keys.

Comment on test_wb_publish_embedded clarifies it only proves not-crashing — a mismatched --db-server would still exit 0 while silently dropping the creds (tracked internally).

Test plan

pytest tests/ → 341 passed, 2 skipped
Unit tests: 4 new
- test_publish_with_db_username_missing_db_server_exits — workbook + --db-username w/o --db-server → friendly exit
- test_publish_with_oauth_username_missing_db_server_exits — workbook + --oauth-username w/o --db-server → friendly exit
- test_publish_with_oauth_creds — positive control (complete oauth creds → no block)
- test_publish_datasource_with_db_username_no_db_server — negative control (datasource path is NOT blocked, workbook-only behavior)
E2E test: 1 new
- test_wb_publish_embedded_missing_db_server_fails — asserts the friendly error message via the CLI

🤖 Generated with Claude Code

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. - Restore the --db-server required-when-embedded guard on the workbook publish path. - Extract _require_db_server_for_workbook(args, filename, logger) helper so the duplicated guard cannot drift between the early pre-auth check and the in-loop per-file check. - Extend the English publish.options.db-server help text to warn about the silent-drop-on-mismatch footgun. - Fix Portuguese grammar: "banco de dado" -> "banco de dados" (plural); "associado a credenciais" -> "associado as credenciais" (crase). - Regenerate .mo catalogs to pick up the new keys. - Unit tests: db-username and oauth-username exit paths, plus datasource-not-blocked coverage. - E2E test: friendly exit when --db-server is missing. Comment on test_wb_publish_embedded clarifies it only proves not-crashing (a mismatched --db-server would still exit 0 while silently dropping the creds; tracked internally). Re-cut from the closed PR #470 stack. Previously PR C in the #467/#469/#470 backport chain; PR B's base branch was deleted after merge, orphaning PR #470. This lands the same content on a fresh branch off development. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

github-actions · 2026-09-18T21:50:19Z

Coverage Report

File	Stmts	Miss	Cover	Missing
tabcmd
__main__.py	12	12	12	0%
_version.py	11	11	11	0%
tabcmd.py	15	15	15	0%
version.py	9	5	5	44%
tabcmd/commands
commands.py	10	10	10	0%
constants.py	77	18	18	77%
server.py	135	18	18	87%
tabcmd/commands/auth
session.py	394	50	50	87%
tabcmd/commands/datasources_and_workbooks
datasources_and_workbooks_command.py	157	18	18	89%
datasources_workbooks_views_url_parser.py	142	5	5	96%
delete_command.py	60	16	16	73%
export_command.py	120	25	25	79%
get_url_command.py	127	47	47	63%
publish_command.py	133	16	16	88%
runschedule_command.py	21	7	7	67%
tabcmd/commands/extracts
create_extracts_command.py	42	8	8	81%
decrypt_extracts_command.py	27	2	2	93%
delete_extracts_command.py	37	6	6	84%
encrypt_extracts_command.py	27	2	2	93%
extracts.py	20	2	2	90%
reencrypt_extracts_command.py	27	2	2	93%
refresh_extracts_command.py	48	10	10	79%
tabcmd/commands/group
create_group_command.py	29	5	5	83%
delete_group_command.py	27	2	2	93%
tabcmd/commands/project
create_project_command.py	46	8	8	83%
delete_project_command.py	35	4	4	89%
publish_samples_command.py	30	4	4	87%
tabcmd/commands/site
create_site_command.py	34	5	5	85%
delete_site_command.py	27	2	2	93%
edit_site_command.py	38	2	2	95%
list_command.py	77	12	12	84%
list_sites_command.py	29	2	2	93%
tabcmd/commands/user
add_users_command.py	29	5	5	83%
create_site_users.py	58	11	11	81%
create_users_command.py	59	9	9	85%
delete_site_users_command.py	43	5	5	88%
user_data.py	222	31	31	86%
tabcmd/execution
_version.py	2	2	2	0%
global_options.py	126	8	8	94%
localize.py	66	11	11	83%
logger_config.py	60	6	6	90%
tabcmd_controller.py	42	7	7	83%
TOTAL	2884	446	85%

Copilot

🟡 Changes recommended

Add the new error translation to all supported locales and ensure E2E tests reject unresolved message keys.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds fail-fast validation requiring --db-server when publishing workbooks with embedded credentials, plus tests and localization updates. A missing translation fallback remains to be addressed.

Changes:

Adds shared workbook validation and user-facing errors.
Adds unit and E2E coverage.
Updates help text and Portuguese translations.

File summaries

File	Summary
`tests/e2e/online_tests.py`	Adds missing-server E2E coverage.
`tests/commands/test_publish_command.py`	Adds workbook and datasource credential tests.
`tabcmd/locales/pt/tabcmd_messages_pt.properties`	Corrects Portuguese wording.
`tabcmd/locales/en/tabcmd_messages_en.properties`	Adds validation and help messages.
`tabcmd/commands/datasources_and_workbooks/publish_command.py`	Adds workbook credential validation.

Review details

Suppressed comments (1)

tests/e2e/online_tests.py:518

This fallback makes the E2E test pass when localization is broken: it explicitly accepts the unresolved msgid, so missing .mo entries for this new error will not be detected. Since the expected behavior is a clear user-facing message, reject the raw key while still requiring the rendered guidance.

        # Localized string OR the raw key (if .mo has not been regenerated yet) both signal our guard.
        combined = (result.stdout or "") + (result.stderr or "")
        assert "publish.errors.db_server_required" in combined or "--db-server is required" in combined, (

Files reviewed: 5/7 changed files
Comments generated: 1
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

 publish.description=Publish a workbook, data source, or extract to the server
 publish.errors.unexpected_server_response=Unexpected response from the server: {0}
 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.errors.db_server_required=--db-server is required when publishing a workbook with --db-username or --oauth-username. Use --db-server to specify the database server address the embedded credentials should be associated with.


…ll locales Copilot flagged the new key as English-only, so users in de/es/fr/ga/ it/ja/ko/pt/sv/zh would see the raw msgid (`publish.errors.db_server_required`) instead of any actual message. Adds the English placeholder text to each of the 10 non-EN properties catalogs and regenerates the .mo bundles via `doit localize` so the rendered guidance appears in every locale until real translations arrive. Tightens the e2e assertion to require the rendered English text (dropping the raw-msgid fallback path) so localization regressions fail loudly instead of silently.

Copilot

Copilot review overview

🟡 Changes recommended

Fix the missing test argument and validate all directory workbooks before publishing to prevent partial success.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

Validate all directory workbooks before publishing any · New
This key is only present in the English properties file; the other supported locale catalogs have…

            if source in ["twbx", "twb"]:
+                # TSC's workbook publish path requires ConnectionItem.server_address whenever
+                # embedded connection credentials are supplied; datasource publish does not.
+                PublishCommand._require_db_server_for_workbook(args, str_filename, logger)


Copilot AI lite review requested due to automatic review settings September 18, 2026 21:49

Copilot started reviewing on behalf of jacalata September 18, 2026 21:50 View session

Copilot AI reviewed Sep 18, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 19, 2026 00:49

Copilot started reviewing on behalf of jacalata September 19, 2026 00:49 View session

Copilot AI reviewed Sep 19, 2026

View reviewed changes

Navigation Menu

Require --db-server when publishing with embedded credentials - #473

Require --db-server when publishing with embedded credentials#473
jacalata wants to merge 2 commits into
developmentfrom
jac/db-server-embed-creds

jacalata commented Sep 18, 2026

Uh oh!

github-actions Bot commented Sep 18, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jacalata commented Sep 18, 2026

Motivation

Behavior change

Test plan

Uh oh!

github-actions Bot commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Sep 18, 2026 •

edited

Loading