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

Bug fix: don't constantly update APIRule in tests while getting the status #1581

Merged

Conversation

mluk-sap
Copy link
Contributor

@mluk-sap mluk-sap commented Jan 3, 2025

Description

Changes proposed in this pull request:

  • Refactored the way of applying apiRules in tests, so the behavior is more explicit and predictable:
    • introduced a function to read single manifest template from yaml file (returns error if there are multiple manifests)
    • simplified APIRule helper functions, so they do what is expected:
      • applyApiRule creates new single ApiRule (returns error if APIRule exists)
      • updateApiRule updates an existing single ApiRule (returns error if APIRule doesn't exist)
      • these functions don't call UpdateResources anymore in order to get APIRule status, which was breaking tests a lot
    • istio-jwt / scenario_noauth_and_jwt - proper installation of two APIRules (previously only one was considered in state analysis)
    • ory / service_per_path - helloworld workload deployed explicitly (previously it was deployed as a side effect of applying APIRule)
  • Reintroduced ScenarioInitializer for all tests, which improves test separation (regenerates random identifiers) and allows to detect cases where tests share some state in an implicit way (previously Custom Domain tests were sharing the same APIRule name)
  • Refactored Custom Domain tests (because of ScenarioInitializer changes):
    • custom domain resources are prepared in BeforeSuiteHooks and cleaned in AfterSuiteHooks
    • scenarios are now separated, so they don't share any state, which is a good practice (previously APIRule was common)
    • improved readability of logs - more logs during preparation, test output is not convoluted by log messages, etc.
    • renamed some variables, so their purpose is more clear (like subdomainForTests)
  • Improved APIRule related error messages, so they contain more context information (like APIRule name)

Pre-Merge Checklist

  • As a PR reviewer, verify code coverage and evaluate if it is acceptable.

Related issues

@mluk-sap mluk-sap requested a review from a team as a code owner January 3, 2025 13:24
@kyma-bot kyma-bot added cla: yes Indicates the PR's author has signed the CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 3, 2025
@mluk-sap mluk-sap force-pushed the dont-update-apirules-during-tests branch from 9e88531 to feed22b Compare January 4, 2025 10:07
@kyma-bot kyma-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 6, 2025
Ressetkk
Ressetkk previously approved these changes Jan 7, 2025
Copy link
Contributor

@Ressetkk Ressetkk left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks.

@kyma-bot kyma-bot added the lgtm Looks good to me! label Jan 7, 2025
tests/integration/pkg/helpers/api_rule.go Outdated Show resolved Hide resolved
tests/integration/pkg/network/dns.go Outdated Show resolved Hide resolved
@kyma-bot kyma-bot removed the lgtm Looks good to me! label Jan 7, 2025
…t/retry-go implementation with fixed delay like in other places
@kyma-bot kyma-bot added the lgtm Looks good to me! label Jan 7, 2025
@mluk-sap mluk-sap merged commit 325a73c into kyma-project:main Jan 8, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates the PR's author has signed the CLA. lgtm Looks good to me! size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants