Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(dependency_resolution): Dashboard-Share-Settings can no longer be referenced from dashboards. #1614

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Laubi
Copy link
Contributor

@Laubi Laubi commented Oct 24, 2024

What this PR does / Why we need it:

Dashboard share settings are a bit special. One dashboard always has exactly 1 dashboard-share config connected to it. This share setting has the same ID as the dashboard itself. This is where the issues arise.

Dashboards can't depend on other dashboards, as we ignore those references. The references are usually only included in the markdown of the tiles, not the dashboards themselves. Since the dashboard share settings have the same ID as the dashboards, what means that instead of ignoring dashboard->dashboard references, we get unwanted dashboard->dashboard-share-setting references. What is more, the references are not within the same config, but some random dashboards can suddenly depend on the dashboard-share-settings.
Since dashboard-share settings do depend on their parent dashboard, it can happen that we suddenly introduce cyclic dependencies where there are actually none.

This fix ignores dashboard-share-settings completely as nothing can actually reference them. They just exist as the children of dashboards.

Special notes for your reviewer:

Changeset contains a small refactoring in extracting common code to shared.go

Does this PR introduce a user-facing change?

Yes, users will have a more stable experiencing when downloading/uploading dynatrace environments

@Laubi Laubi requested a review from a team as a code owner October 24, 2024 16:21
Copy link

github-actions bot commented Oct 24, 2024

Unit Test Results

1 903 tests  +2   1 902 ✅ +2   1m 56s ⏱️ ±0s
  134 suites ±0       1 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit 4e643dc. ± Comparison against base commit 5323f58.

♻️ This comment has been updated with latest results.

@Laubi Laubi added the release-notes This feature/fix should be mentioned in release notes label Oct 24, 2024
Copy link

github-actions bot commented Oct 24, 2024

E2E Test Results

    4 files   -   1    268 suites   - 133   26m 58s ⏱️ - 40m 41s
2 003 tests ±  0  2 001 ✅ ±  0  2 💤 ±0  0 ❌ ±0 
2 118 runs   - 115  2 116 ✅  - 115  2 💤 ±0  0 ❌ ±0 

Results for commit ce27955. ± Comparison against base commit 2298653.

This pull request removes 2 and adds 2 tests. Note that renamed tests count towards both.
github.com/dynatrace/dynatrace-configuration-as-code/v2/cmd/monaco/integrationtest/v2 ‑ TestPaginationClassic
github.com/dynatrace/dynatrace-configuration-as-code/v2/cmd/monaco/integrationtest/v2 ‑ TestPaginationPlatform
github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/download/dependency_resolution ‑ TestDependencyResolution/Dashboards_should_not_be_able_to_reference_a_dashboard-share-setting,_even_if_it's_the_dashboard's_share_setting_BasicResolver
github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/download/dependency_resolution ‑ TestDependencyResolution/Dashboards_should_not_be_able_to_reference_a_dashboard-share-setting,_even_if_it's_the_dashboard's_share_setting_FastResolver

♻️ This comment has been updated with latest results.

@Laubi Laubi added the run-e2e-test Manually trigger the E2E tests for reviewed PRs label Oct 28, 2024
@Laubi Laubi force-pushed the fix/dashboard-share-settings-can-not-be-referenced branch from 1ba5f2d to ce27955 Compare October 28, 2024 12:10
@Laubi Laubi added run-e2e-test Manually trigger the E2E tests for reviewed PRs and removed run-e2e-test Manually trigger the E2E tests for reviewed PRs labels Oct 28, 2024
@Laubi Laubi force-pushed the fix/dashboard-share-settings-can-not-be-referenced branch from ce27955 to 139edc0 Compare October 28, 2024 12:30
… referenced from dashboards.

Dashboard share settings are a bit special. One dashboard always has exactly 1 dashbaord share setting connected to it. This share setting has the same ID as the dashboard itself. This is were the issues arise.

Dashboards can't depend on other dashboards, as we ignore those references. They are usually only included in the markdown of the tiles, not the dashboards themselves. Since the dashboard share settings have the same ID as the dashboards, what means that instead of ignoring dashboard->dashboard references, we get unwanted dashboard->dashboard-share-settings references. What is more, the references are not within the same config, but some random dashboards can suddenly depend on the dashboard-share-settings.
Since dashboard-share settings do depend on their parent dashboard, it can happen that we suddenly introduce unwanted cyclic dependencies.

This fix ignores dashboard-share-settings completely as nothing can actually reference them.
@Laubi Laubi force-pushed the fix/dashboard-share-settings-can-not-be-referenced branch from 139edc0 to 4e643dc Compare October 30, 2024 13:14
Copy link

sonarcloud bot commented Oct 30, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes This feature/fix should be mentioned in release notes run-e2e-test Manually trigger the E2E tests for reviewed PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants