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

feat: added timeout as attribute to checks #834

Merged
merged 2 commits into from
Jul 27, 2023

Conversation

bartvdbraak
Copy link
Contributor

@bartvdbraak bartvdbraak commented Jul 19, 2023

All Submissions:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My code follows the code style of this project.
  • I ran lint checks locally prior to submission.
  • Have you checked to ensure there aren't other open PRs for the same update/change?

What about the current behavior has changed?

While working on Pull Request #830, I noticed that some checks were missing the timeout attribute.

If you look at the API request specification and the GUI on Azure DevOps, it becomes clear that this is part of their spec and has a certain default and a max size of a 32-bit integer:

Example cURL for Branch Control check

curl \
  'https://dev.azure.com/{organization}/{project}/_apis/pipelines/checks/configurations?api-version=7.0-preview.1' \
  -X POST \
  --data-raw '
    {
      "type": {
        "id": "fe1de3ee-a436-41b4-bb20-f6eb4cb879a7",
        "name": "Task Check"
      },
      "settings": {
        "definitionRef": {
          "id": "86b05a0c-73e6-4f7d-b3cf-e38f3b39a75b",
          "name": "evaluatebranchProtection",
          "version": "0.0.1"
        },
        "displayName": "Branch control",
        "inputs": {
          "allowedBranches": "*",
          "ensureProtectionOfBranch": "false"
        },
        "retryInterval": 0,
        "linkedVariableGroup": null
      },
      "resource": {
        "type": "environment",
        "id": "{environmentId}",
        "name": "{environmentName}"
      },
      "timeout": 50000
    }'

Example cURL for Business Hours check

curl \
  'https://dev.azure.com/{organization}/{project}/_apis/pipelines/checks/configurations?api-version=7.0-preview.1' \
  -X POST \
  --data-raw '
    {
      "type": {
        "id": "fe1de3ee-a436-41b4-bb20-f6eb4cb879a7",
        "name": "Task Check"
      },
      "settings": {
        "definitionRef": {
          "id": "445fde2f-6c39-441c-807f-8a59ff2e075f",
          "name": "evaluateBusinessHours",
          "version": "0.0.1"
        },
        "displayName": "Business Hours",
        "inputs": {
          "businessDays": "Monday,Tuesday,Wednesday,Thursday,Friday",
          "timeZone": "UTC",
          "startTime": "04:00",
          "endTime": "11:00"
        },
        "retryInterval": 0,
        "linkedVariableGroup": null
      },
      "resource": {
        "type": "environment",
        "id": "{environmentId}",
        "name": "{environmentName}"
      },
      "timeout": 2147483647
    }'

GUI Examples in Azure DevOps
Screenshot 2023-07-20 at 00 18 04
Screenshot 2023-07-20 at 00 18 44

Issue Number:

Does this introduce a change to go.mod, go.sum or vendor/?

  • No

Does this introduce a breaking change?

  • No

Any relevant logs, error output, etc?

n/a

Other information

n/a

Copy link
Collaborator

@xuzhang3 xuzhang3 left a comment

Choose a reason for hiding this comment

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

Please ensure all the AccTests passed

@bartvdbraak
Copy link
Contributor Author

Please ensure all the AccTests passed

Can you show me what is failing in the Acceptance tests?

Currently I see that all checks have passed:
Screenshot 2023-07-26 at 14 16 00

@xuzhang3
Copy link
Collaborator

@bartvdbraak The AccTest has been fixed by the latest commit. The pipeline only run the unit tests not AccTests.
Error log

=== CONT  TestAccCheckBranchControl_complete
    resource_check_branch_control_test.go:48: Step 1/1 error: Check failed: Check 5/7 error: azuredevops_check_branch_control.test: Attribute 'timeout' expected "50000", got "1440"
=== CONT  TestAccCheckBranchControl_basic
    resource_check_branch_control_test.go:22: Step 1/1 error: Check failed: Check 5/5 error: azuredevops_check_branch_control.test: Attribute 'timeout' expected "50000", got "1440"

@xuzhang3
Copy link
Collaborator

=== RUN   TestAccCheckBranchControl_basic
=== PAUSE TestAccCheckBranchControl_basic
=== RUN   TestAccCheckBranchControl_complete
=== PAUSE TestAccCheckBranchControl_complete
=== RUN   TestAccCheckBranchControl_update
=== PAUSE TestAccCheckBranchControl_update
=== CONT  TestAccCheckBranchControl_basic
=== CONT  TestAccCheckBranchControl_update
=== CONT  TestAccCheckBranchControl_complete
--- PASS: TestAccCheckBranchControl_complete (80.09s)
--- PASS: TestAccCheckBranchControl_basic (80.43s)
--- PASS: TestAccCheckBranchControl_update (101.69s)
PASS
ok      github.com/microsoft/terraform-provider-azuredevops/azuredevops/internal/acceptancetests        108.609s

=== RUN   TestAccCheckBusinessHours_basic
=== PAUSE TestAccCheckBusinessHours_basic
=== RUN   TestAccCheckBusinessHours_complete
=== PAUSE TestAccCheckBusinessHours_complete
=== RUN   TestAccCheckBusinessHours_update
=== PAUSE TestAccCheckBusinessHours_update
=== CONT  TestAccCheckBusinessHours_basic
=== CONT  TestAccCheckBusinessHours_complete
=== CONT  TestAccCheckBusinessHours_update
--- PASS: TestAccCheckBusinessHours_complete (98.87s)
--- PASS: TestAccCheckBusinessHours_basic (98.93s)
--- PASS: TestAccCheckBusinessHours_update (128.72s)
PASS
ok      github.com/microsoft/terraform-provider-azuredevops/azuredevops/internal/acceptancetests        130.944s

@xuzhang3 xuzhang3 merged commit fb2e284 into microsoft:main Jul 27, 2023
3 checks passed
@xuzhang3 xuzhang3 added this to the v0.8.0 milestone Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants