Skip to content

fix(gooddata-sdk): keep isHiddenFromKda on declarative metrics - #1891

Merged
tychtjan merged 1 commit into
masterfrom
jtyc/gdai-2630
Oct 21, 2026
Merged

tychtjan merged 1 commit into
masterfrom
jtyc/gdai-2630

Conversation

@tychtjan

@tychtjan tychtjan commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Summary

CatalogDeclarativeMetric didn't declare is_hidden_from_kda. The generated DeclarativeMetric already has it, but cattrs drops any key the attrs class doesn't declare. So the SDK silently lost the flag on every load and store of the analytics model:

  • a layout from git with isHiddenFromKda: true reached the server without it
  • a load-then-put wiped a flag set in the UI

This adds the field to the metric class only (CatalogAnalyticsBase is shared by dashboards, plugins and others whose models don't have it). An unset field stays None and isn't sent, so existing layouts and cassettes don't change.

FI needs this to hide the Benchmark (bmk_*) metrics from KDA "Explain" via a git deploy (MC-5594). The AAC (gdc deploy) format has no KDA key yet; that's a separate gdc-ui change and isn't part of this PR.

Test plan

  • New test_declarative_analytics_metrics.py: true / false / absent through store_to_disk → load_from_disk → to_api(). Master: 3 failed. Branch: 3 passed.
  • make -C packages/gooddata-sdk format lint type-check: clean
  • TEST_ENVS=py314 make -C packages/gooddata-sdk test: 576 passed, 2 skipped, 3 xfailed (no cassette re-recorded)
  • TEST_ENVS=py314 make -C packages/gooddata-pipelines test: 195 passed
  • Local gdc-nas stack, throwaway workspace, value read back with a raw layout GET:
Step master branch
SDK put with true absent true
UI-style raw PUT true, then SDK get + put absent (wiped) true
SDK put without the key absent absent
  • py310–py313 (CI matrix)

Risk

low: one optional field on one class, default None and never serialized when unset.

jira: GDAI-2630

Summary by CodeRabbit

  • New Features
    • Declarative metrics can now indicate whether they are hidden from the KPI dashboard area.
    • The setting supports enabled, disabled, or unspecified values and is preserved when metrics are serialized and restored.

@coderabbitai

coderabbitai Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 938e0127-717b-4296-a692-d8acdd56a11d
📥 Commits

Reviewing files that changed from the base of the PR and between f79f0ef and ac60411.

📒 Files selected for processing: 1
  • packages/gooddata-sdk/tests/catalog/unit_tests/test_declarative_analytics_metrics.py

Included review availability: This review used your included allowance. 2 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.

📝 Walkthrough

The declarative metric model adds an optional is_hidden_from_kda field. A parametrized test checks true, false, and absent values across storage, deserialization, and API serialization.

Changes: Metric KDA visibility

Layer / File(s) Summary
Metric field and round-trip validation
packages/gooddata-sdk/src/gooddata_sdk/catalog/workspace/declarative_model/workspace/analytics_model/analytics_model.py, packages/gooddata-sdk/tests/catalog/unit_tests/test_declarative_analytics_metrics.py
CatalogDeclarativeMetric adds is_hidden_from_kda, which defaults to None. Tests check true, false, and absent values through storage, deserialization, and API serialization.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to ac604

The KDA visibility field is correctly preserved across declarative metric round trips, with absent values omitted.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: preserving isHiddenFromKda on declarative metrics.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

A rabbit checks the metric field
True and false pass through the yield
When absent, None is what we find
The saved API shape stays aligned
I nibble clover, pleased and mild

Comment @coderabbitai help to get the list of available commands.

CatalogDeclarativeMetric did not declare is_hidden_from_kda. The
generated DeclarativeMetric gained the field when the API client was
regenerated, but the SDK attrs class was not updated, and cattrs drops
keys a class does not declare. So loading a metric layout from disk or
the server lost the flag silently, and every SDK put sent the metric
without it: a git deploy could never set it, and a load-then-put wiped
a flag set in the UI.

Declare the field on the metric class only; the other analytics
classes share CatalogAnalyticsBase but their generated models have no
such field. An unset field stays None and is not sent, so existing
layouts and cassettes are unchanged. A round-trip test covers true,
false and an absent key through disk and to_api().

jira: GDAI-2630
risk: low
@codecov

codecov Bot commented Oct 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.09%. Comparing base (9e9ffae) to head (ac60411).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1891   +/-   ##
=======================================
  Coverage   85.09%   85.09%           
=======================================
  Files         337      337           
  Lines       24331    24332    +1     
=======================================
+ Hits        20704    20705    +1     
  Misses       3627     3627           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tychtjan
tychtjan merged commit d2065c2 into master Oct 21, 2026
14 checks passed
@tychtjan
tychtjan deleted the jtyc/gdai-2630 branch October 21, 2026 13:37
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.

2 participants