Skip to content

--db-server validation + e2e tests + review polish - #470

Open
jacalata wants to merge 9 commits into
jac/regen-mo-post-458from
jac/publish-embed-creds-tests
Open

--db-server validation + e2e tests + review polish#470
jacalata wants to merge 9 commits into
jac/regen-mo-post-458from
jac/publish-embed-creds-tests

Conversation

@jacalata

@jacalata jacalata commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Third in the #467 backport-split (PR C). Stacked on top of PR B (jac/regen-mo-post-458, #469).

What this does

Adds the follow-up work that was reverted out of PR A (#467) to keep the backport clean:

  • Restores the --db-server required-when---db-username guard on the workbook publish path.
  • Restores unit tests covering both the db-username and oauth-username exit paths, plus datasource-not-blocked coverage.
  • Restores the e2e negative test asserting the friendly exit when --db-server is missing.

Plus fresh-eyes review Medium fixes and one Copilot nit:

  • M2: 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.
  • M3: Add a comment on test_wb_publish_embedded clarifying it only proves not-crashing (a mismatched --db-server would still exit 0 while silently dropping the creds; tracked internally).
  • M4: 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).
  • Re-regenerated .mo catalogs to pick up the M4 and PT changes.

Stacked-PR notes

Generated with Claude Code

jacalata and others added 9 commits September 10, 2026 14:42
@jacalata @claude
Require --db-server when publishing with embedded credentials
c138a1c 
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>
@jacalata @claude
Add e2e coverage for --db-server requirement
53d7c5d 
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>
@jacalata @claude
Address code-review findings on the --db-server guard
9c64d74 
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>
@jacalata @claude
Extract _require_db_server_for_workbook helper (M2)
806c53a 
The guard against workbook publish with embedded credentials but no
--db-server was duplicated in run_command (early pre-auth check and
in-loop per-file check). Extract it into a single classmethod so the
two call sites cannot drift.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jacalata @claude
Comment on what test_wb_publish_embedded actually asserts (M3)
f2a97fb 
The e2e test only proves the publish call did not crash. It does not
verify credentials actually embedded on the server side; a mismatched
--db-server would exit 0 while silently dropping the creds at TSC's
request-factory boundary. Document the limitation so future readers do
not mistake it for full coverage.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jacalata @claude
Extend --db-server help text with silent-drop caveat (M4)
2a54068 
Callers who pass --db-username without --db-server now get a friendly
error. But callers who pass a mismatched --db-server still hit the
silent-drop bug at the TSC boundary. Document the mismatch footgun in
the help text so users notice before they publish.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jacalata @claude
Fix Portuguese grammar on publish.options.db-server
df9f6c8 
Two small corrections in the pt locale:
- "banco de dado" -> "banco de dados" (plural is the standard form)
- "associado as credenciais" -> "associado as credenciais" (crase, "a"+"as")

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jacalata @claude
Regenerate .mo files after help-text updates
fd661c2 
Re-runs doit localize so en (M4 caveat added) and pt (grammar fixed)
message catalogs match their .properties sources.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jacalata @claude
Drop internal work-item reference from public test comment
ac40b1e 
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jacalata jacalata mentioned this pull request Sep 10, 2026
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yyk16293 yyk16293 yyk16293 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jacalata @yyk16293