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: add support for run task results callback #867
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for submitting this PR & sorry for the delay in reviewing. Some things to chew on in the meantime ⬇️
run_task_request.go
Outdated
const ( | ||
// RunTaskRegistrationRunId is the string that TFC/E sends when it just wants to register the Run Task. | ||
RunTaskRegistrationRunId = "run-xxxxxxxxxxxxxxxx" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to understand this more, along with the IsEndpointValidation
check. Is the intent to ensure r.RunID
is a valid run external ID (in the format of RunTaskRegistrationRunID
)? As I read it, IsEndpointValidation
will only return true if r.RunID == "run-xxxxxxxxxxxx"
which should never be the case.
If we do want to ensure that it's in proper external ID format, use the helper validStringID()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you create a run task, you are asked to provide an Endpoint URL. TFE sends a payload to it to make sure that it is a valid endpoint (have not found any relevant docs on this).
I got that function from the same function that is used by HashiCorp in the scaffolding repo. There, the IsEndpointValidation
function checks against the token of the TFE request payload which will be test-token only when you first create the run task.
This is the payload from the above TFE request:
{
"access_token": "test-token",
"capabilities": {
"outcomes": true
},
"configuration_version_download_url": ".../api/v2/configuration-versions/cv-xxxxxxxxxxxxxxxx/download",
"configuration_version_id": "cv-xxxxxxxxxxxxxxxx",
"is_speculative": false,
"organization_name": "test-org",
"payload_version": 1,
"plan_json_api_url": ".../api/v2/runs/plan-xxxxxxxxxxxxxxxx/plan/json-output",
"run_app_url": ".../app/test-org/test-workspace/runs/run-xxxxxxxxxxxxxxxx",
"run_created_at": "2021-01-01T00:00:00.000Z",
"run_created_by": "test-user",
"run_id": "run-xxxxxxxxxxxxxxxx",
"run_message": "Test run message",
"stage": "test",
"task_result_callback_url": ".../api/v2/task-results/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx/callback",
"task_result_enforcement_level": "test",
"task_result_id": "taskrs-xxxxxxxxxxxxxxxx",
"vcs_branch": null,
"vcs_commit_url": null,
"vcs_pull_request_url": null,
"vcs_repo_url": null,
"workspace_app_url": "...",
"workspace_id": "ws-xxxxxxxxxxxxxxxx",
"workspace_name": "test-workspace",
"workspace_working_directory": ""
}
I can change the boolean check to check the token instead of the run ID as shown in the hashicorp/terraform-run-task-scaffolding-go repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the validation to using the test token that TFC/E sends to register the endpoint as a run task. Let me know what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the implementation look great ✅ However I do think we ought to add some unit tests around this behavior, particularly ensuring that things are being serialized correctly (TaskResultTag
gives me some pause given mix-&-matching of json/jsonapi
struct tags).
Writing a true integration test would not be plausible here, as it would require the test function to expose a publicly available server.
What sort of unit tests might be possible here?
- Test for validation errors, scenarios that would result in
ErrInvalidTaskResultsCallbackStatus
,ErrInvalidCallbackURL
,ErrInvalidAccessToken
andErrInvalidTaskResultsCallbackType
. - Ensure the
Authorization
header is set to the correct token provided in the request sent from TFC. In the test function, you can spin up an httptest.Server, define an endpoint that validates the header is the correct token, and set that endpoint URL toTaskResultCallbackURL
in a mockedRunTaskRequest
. - Ensure
TaskResultTag
is serialized correctly.
Last thing, make sure you define the server in helper_test.go
. An example signature can be:
func runTaskCallbackMockServer(t *testing.T) *httptest.Server {}
Once you've added tests, make sure to run them locally and add the test output to the PR description. CI won't fire given this is a fork's branch, so once everything is a-ok I will rebase this PR onto a local branch to trigger CI.
// TaskResultCallbackRequestOptions represents the TFC/E Task result callback request | ||
// https://developer.hashicorp.com/terraform/enterprise/api-docs/run-tasks/run-tasks-integration#request-body-1 | ||
type TaskResultCallbackRequestOptions struct { | ||
Type string `jsonapi:"primary,task-results"` | ||
Status TaskResultStatus `jsonapi:"attr,status"` | ||
Message string `jsonapi:"attr,message,omitempty"` | ||
URL string `jsonapi:"attr,url,omitempty"` | ||
Outcomes []*TaskResultOutcome `jsonapi:"relation,outcomes,omitempty"` | ||
} | ||
|
||
// TaskResultOutcome represents a detailed TFC/E run task outcome, which improves result visibility and content in the TFC/E UI. | ||
// https://developer.hashicorp.com/terraform/enterprise/api-docs/run-tasks/run-tasks-integration#outcomes-payload-body | ||
type TaskResultOutcome struct { | ||
Type string `jsonapi:"primary,task-result-outcomes"` | ||
OutcomeID string `jsonapi:"attr,outcome-id,omitempty"` | ||
Description string `jsonapi:"attr,description,omitempty"` | ||
Body string `jsonapi:"attr,body,omitempty"` | ||
URL string `jsonapi:"attr,url,omitempty"` | ||
Tags map[string][]*TaskResultTag `jsonapi:"attr,tags,omitempty"` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move these above the Update()
call for visibility. The style ordering for a file is:
- Interface definition
- Struct definitions
- Receiver method implementations.
Description
This PR aims to address the missing support for Run Tasks Integration API. I have already raised an issue #862 about this.
It introduces the following:
Testing plan
Unfortunately, it is not easy to test the Update function because you first need a running server which listens for incoming TFC/E requests. Then, you need a workspace run which is configured to use that run task endpoint.
External links
https://github.com/hashicorp/terraform-run-task-scaffolding-go this HashiCorp repo contains part of the code that can be found here.