Skip to content
New issue

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

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

Already on GitHub? # to your account

fix: validator checking unique names doesn't return early anymore on … #1807

Merged

Conversation

d0weinberger
Copy link
Contributor

What this PR does / Why we need it:

This fixes a bug where we erroneously return early from unique name validation for classic apis if the scope of the config-to-check differs from a scope of the configs we have already seen before.

YAML to validate

configs:
  - id: settings-1
    config:
      name: Duplicated
      template: dashboard-share-settings.json
      skip: false
    type:
      api:
        name: dashboard-share-settings
        scope:
          type: value
          value: scope-0
  - id: settings-2
    config:
      name: Duplicated
      template: dashboard-share-settings.json
      skip: false
    type:
      api:
        name: dashboard-share-settings
        scope:
          type: value
          value: scope-1
  - id: settings-3
    config:
      name: Duplicated
      template: dashboard-share-settings.json
      skip: false
    type:
      api:
        name: dashboard-share-settings
        scope:
          type: value
          value: scope-1

Current (wrong) log output:

2025-03-25T11:17:51+01:00	info	Deploying configurations to environment "platform_env"...
2025-03-25T11:17:51+01:00	info	Deploying 3 independent configuration sets in parallel...
2025-03-25T11:17:51+01:00	info	[coord=test:dashboard-share-settings:settings-3][gid=2]	Deploying config
2025-03-25T11:17:51+01:00	info	[coord=test:dashboard-share-settings:settings-1][gid=0]	Deploying config
2025-03-25T11:17:51+01:00	info	[coord=test:dashboard-share-settings:settings-2][gid=1]	Deploying config

Expected log output, introduced with this PR:

2025-03-25T11:30:40+01:00	error	Error: Deployment failed - check logs for details: Errors encountered for 1 environment(s):
	"platform_env": duplicated config name found: configurations test:dashboard-share-settings:settings-3 and test:dashboard-share-settings:settings-2 define the same 'name' "Duplicated"

Special notes for your reviewer:

A test has been added to check the behavior. If you execute the test without the fix, it will fail.

Does this PR introduce a user-facing change?

Yes, validation will fail now for non-unique names in the same scope, whereas before there were cases where validation would succeed, although it shouldn't.

@d0weinberger d0weinberger requested a review from a team as a code owner March 25, 2025 12:11
@d0weinberger d0weinberger added the run-e2e-test Manually trigger the E2E tests for reviewed PRs label Mar 25, 2025
Copy link

github-actions bot commented Mar 25, 2025

Unit Test Results

1 943 tests  +1   1 942 ✅ +1   53s ⏱️ ±0s
  134 suites ±0       1 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit 8789145. ± Comparison against base commit 5221b02.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Mar 25, 2025

E2E Test Results

    3 files   -   1    269 suites   - 134   26m 59s ⏱️ - 53m 43s
2 048 tests  -   1  2 032 ✅  -   2  2 💤 ±0  14 ❌ +1 
2 227 runs   - 181  2 211 ✅  - 182  2 💤 ±0  14 ❌ +1 

For more details on these failures, see this check.

Results for commit 894fbe0. ± Comparison against base commit f31a9f7.

This pull request removes 2 and adds 1 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/deploy/internal/classic ‑ TestValidate_ErrorForSameNameAndScopeWithDifferentScopeCheckedEarlier

♻️ This comment has been updated with latest results.

@d0weinberger d0weinberger force-pushed the fix/classic-validator-early-returns-on-different-scopes branch from f90f464 to 894fbe0 Compare March 25, 2025 13:41
@d0weinberger d0weinberger 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 Mar 25, 2025
@d0weinberger d0weinberger force-pushed the fix/classic-validator-early-returns-on-different-scopes branch from 1539adf to 451fef5 Compare March 27, 2025 09:41
Copy link

@d0weinberger d0weinberger force-pushed the fix/classic-validator-early-returns-on-different-scopes branch from 451fef5 to 94cb432 Compare March 27, 2025 10:18
@d0weinberger d0weinberger force-pushed the fix/classic-validator-early-returns-on-different-scopes branch from 94cb432 to 8789145 Compare March 27, 2025 12:20
// If they can't be resolved, we compare the unresolved NameParameters of the two configs,
// first assuming both being of type ReferenceParameter, and if that fails, assuming both being of type CompoundParameter.
// If the fields are equal, a name clash is guaranteed.
// This check is not perfect. Name clashes are not caught if, e.g., one config is resolvable and the other one isn't.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

10/10 comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx!

@d0weinberger d0weinberger merged commit 4b8e0cc into main Mar 27, 2025
10 checks passed
@d0weinberger d0weinberger deleted the fix/classic-validator-early-returns-on-different-scopes branch March 27, 2025 16:31
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
run-e2e-test Manually trigger the E2E tests for reviewed PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants