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

Add Org and Workspace Run Tasks resources #488

Merged
merged 10 commits into from
Jun 14, 2022
Merged

Add Org and Workspace Run Tasks resources #488

merged 10 commits into from
Jun 14, 2022

Conversation

glennsarti
Copy link
Contributor

@glennsarti glennsarti commented May 5, 2022

Description

This PR adds the data.tfe_workspace_run_task and
resource.tfe_workspace_run_task resources to the provider. These
resources will be able to manage Workspace level Run Tasks.

This PR adds the data.tfe_organization_run_task and
resource.tfe_organization_run_task resources to the provider. These
resources will be able to manage Organizational level Run Tasks.

Testing plan

N/A Included Acceptance Tests

External links

Output from acceptance tests

Acceptance TFC instance isn't setup for Run Tasks yet. Manual tests against my local environment are detailed in the comments

It is is now!

@glennsarti glennsarti self-assigned this May 5, 2022
@glennsarti glennsarti requested a review from a team May 5, 2022 07:17
@glennsarti
Copy link
Contributor Author

glennsarti commented May 6, 2022

Org. Run Tasks

?       github.com/hashicorp/terraform-provider-tfe     [no test files]
=== RUN   TestAccTFEOrganizationRunTaskDataSource_basic
--- PASS: TestAccTFEOrganizationRunTaskDataSource_basic (15.67s)
PASS
ok      github.com/hashicorp/terraform-provider-tfe/tfe 16.438s
?       github.com/hashicorp/terraform-provider-tfe/version     [no test files]
?       github.com/hashicorp/terraform-provider-tfe     [no test files]
=== RUN   TestAccTFEOrganizationRunTask_create
--- PASS: TestAccTFEOrganizationRunTask_create (35.66s)
=== RUN   TestAccTFEOrganizationRunTask_import
--- PASS: TestAccTFEOrganizationRunTask_import (24.14s)
PASS
ok      github.com/hashicorp/terraform-provider-tfe/tfe 60.364s
?       github.com/hashicorp/terraform-provider-tfe/version     [no test files]

Workspace Run Tasks

?       github.com/hashicorp/terraform-provider-tfe     [no test files]
=== RUN   TestAccTFEWorkspaceRunTaskDataSource_basic
--- PASS: TestAccTFEWorkspaceRunTaskDataSource_basic (28.94s)
PASS
ok      github.com/hashicorp/terraform-provider-tfe/tfe 29.535s
?       github.com/hashicorp/terraform-provider-tfe/version     [no test files]
?       github.com/hashicorp/terraform-provider-tfe     [no test files]
=== RUN   TestAccTFEWorkspaceRunTask_create
--- PASS: TestAccTFEWorkspaceRunTask_create (33.60s)
=== RUN   TestAccTFEWorkspaceRunTask_import
--- PASS: TestAccTFEWorkspaceRunTask_import (26.05s)
PASS
ok      github.com/hashicorp/terraform-provider-tfe/tfe 60.206s
?       github.com/hashicorp/terraform-provider-tfe/version     [no test files]

@glennsarti glennsarti force-pushed the gs/add-run-tasks branch 2 times, most recently from 4790231 to ba6ed79 Compare May 6, 2022 06:06
Copy link
Collaborator

@brandonc brandonc left a comment

Choose a reason for hiding this comment

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

We'll need some docs created under website/docs for the new resources/data sources

tfe/data_source_organization_run_task_test.go Show resolved Hide resolved
@glennsarti glennsarti force-pushed the gs/add-run-tasks branch 5 times, most recently from 5891ecc to 3d2f497 Compare May 26, 2022 04:08
@glennsarti glennsarti changed the title Add Org and Workspace Run Tasks resources {DO NOT MERGE} Add Org and Workspace Run Tasks resources May 26, 2022
@glennsarti
Copy link
Contributor Author

Lint errors fixed...now for docs

Copy link
Contributor

@sebasslash sebasslash left a comment

Choose a reason for hiding this comment

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

Overall things are looking good. I'm going to need to give it a second pass and make sure I didn't overlook any of the fine print! 😆 Hopefully I have given you enough to chew on!

One really important thing to mention is to include the documentation pages as well!

TESTS.md Show resolved Hide resolved
tfe/resource_tfe_organization_run_task.go Outdated Show resolved Hide resolved
tfe/resource_tfe_organization_run_task.go Show resolved Hide resolved
tfe/resource_tfe_organization_run_task.go Outdated Show resolved Hide resolved
tfe/resource_tfe_workspace_run_task_test.go Show resolved Hide resolved
tfe/resource_tfe_workspace_run_task_test.go Outdated Show resolved Hide resolved
tfe/testing.go Outdated Show resolved Hide resolved
tfe/run_task_helpers_test.go Show resolved Hide resolved
@glennsarti
Copy link
Contributor Author

One really important thing to mention is to include the documentation pages as well!

Yup I know...

This commit removes redundant trailing whitespace.
Some tests require an actual Run Tasks service to be running for
acceptance tests to complete.  This commit adds a new environment
variable `RUN_TASKS_URL` which will be used when creating
legitimate Run Tasks. This commit also adds a testing helper method
to skip tests that require the URL if it is not set.
This commit adds a helper functions `fetchOrganizationRunTask` and
`fetchWorkspaceRunTask` which will be used in later commits to be able
to find a Run Task by name in an organization or workspace.

This commit also adds unit tests for helper and uses uses the mocking
provided by the go-tfe module.

Later commits will migrate the rest of the project to gomocks.
This commit adds the `data.tfe_organization_run_task` and
`resource.tfe_organization_run_task` resources to the provider. These
resources will be able to manage Organizational level Run Tasks.

This commit also includes acceptance tests for these new resources.
This commit adds the `data.tfe_workspace_run_task` and
`resource.tfe_workspace_run_task` resources to the provider. These
resources will be able to manage Workspace level Run Tasks.

This commit also includes acceptance tests for these new resources.
This commit updates the Run Tasks additional code with fixes for
multiple linting errors.
Previously the URL was not validated at TF run time. This commit adds a
basic validator to the URL and adds unit tests to confirm the validation.
@glennsarti glennsarti changed the title {DO NOT MERGE} Add Org and Workspace Run Tasks resources Add Org and Workspace Run Tasks resources Jun 7, 2022
@glennsarti
Copy link
Contributor Author

@sebasslash All ready for re-review

@glennsarti
Copy link
Contributor Author

CI is now running the Run Task acceptance tests

e.g.

PASS tfe.TestAccTFEOrganizationRunTask_validateSchemaAttributeUrl (1.33s)
PASS tfe.TestAccTFEOrganizationRunTask_create (7.47s)
PASS tfe.TestAccTFEOrganizationRunTask_import (6.11s

@glennsarti
Copy link
Contributor Author

Tests are green 💚

Copy link
Contributor

@sebasslash sebasslash left a comment

Choose a reason for hiding this comment

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

We're in the last stretch of the race! Implementation and testing looks really solid; really neat to see the go-tfe mocks in use. Some minor things below on the documentation! Once addressed, this PR has my approval.

environment:
# Note that https://httpstat.us/200 is enough to test CRUD operations on Run Tasks
# themselves BUT any Runs that use the Run Task will timeout and error
RUN_TASKS_URL: "https://httpstat.us/200"
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥 So awesome. Thanks for this Glenn

"url": {
Type: schema.TypeString,
Required: true,
ValidateFunc: validation.IsURLWithHTTPorHTTPS,
Copy link
Contributor

Choose a reason for hiding this comment

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

Also really nice, I believe in another PR i had suggested url.Parse() as the validation mechanism for URLs, did not know it was included in SDKv2.

Copy link
Contributor Author

@glennsarti glennsarti Jun 10, 2022

Choose a reason for hiding this comment

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

Yup, behind the scenes it's just url.parse. But I'd rather use a SDK helper 😃

website/docs/d/workspace_run_task.html.markdown Outdated Show resolved Hide resolved
website/docs/r/organization_run_task.html.markdown Outdated Show resolved Hide resolved

[Run tasks](https://www.terraform.io/cloud-docs/workspaces/settings/run-tasks) allow Terraform Cloud to interact with external systems at specific points in the Terraform Cloud run lifecycle. Run tasks are reusable configurations that you can attach to any workspace in an organization

The tfe_organization_run_task resource creates, updates and destroys Run tasks.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rephrase this. Typically we give a little more context about the resource if necessary. In this case, we should make the logical distinction between what a Workspace and Organization run task are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So rather than that, I've adding links to our documentation as it's always up to date and there's a lot of context there which is too big for the provider docs.

This commit adds documentation for the newly provider data and resource objects.
This commit adds the required Environment Variable to the CI Pipeline which
will then execute the Run Task tests in CI.

Note that https://httpstat.us/200 is enough to test CRUD operations on Run
Tasks themselves BUT any Runs that use the Run Task will timeout and error.
Copy link
Contributor

@sebasslash sebasslash left a comment

Choose a reason for hiding this comment

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

🚀 🎸 🔥

Copy link
Collaborator

@brandonc brandonc left a comment

Choose a reason for hiding this comment

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

Just one question about hmac-key. I'm running through some smoke tests now

"hmac_key": {
Type: schema.TypeString,
Sensitive: true,
Default: "",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Noticing that the API has auditing conditions to handle a nil hmac_key (also nil is allowed in the db). Should the default be nil instead?

Copy link
Contributor Author

@glennsarti glennsarti Jun 13, 2022

Choose a reason for hiding this comment

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

I've done some digging and I don't think I can. In the TF provider SDK, the TypeString type always returns a string. And seems to consider a nil/null value as "" e.g. a zero or nil value for a TypeString is ""

https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/schema/schema.go#L2351

So end result is, even if I set hmac_key => nil or use the default as nil, it always converts that an empty string and there's nothing I can do about it.

This isn't the fault of the API or go-tfe, but rather TF internals.

Copy link
Contributor Author

@glennsarti glennsarti Jun 13, 2022

Choose a reason for hiding this comment

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

The question is then asked... if TypeString is not correct, what type should I be using? the answer is, I don't think there is a better option. The only one that supports nil-able is TypeSet.

tfe/resource_tfe_organization_run_task.go Show resolved Hide resolved
Copy link
Collaborator

@brandonc brandonc left a comment

Choose a reason for hiding this comment

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

Smoke tested ✅

Copy link
Collaborator

@brandonc brandonc left a comment

Choose a reason for hiding this comment

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

@glennsarti I think I made a mistake with how rails is handling this because hmac_key appears to be nil unless I set it to an actual string. Thanks for entertaining my ideas, nonetheless!

@glennsarti glennsarti merged commit 59a346f into main Jun 14, 2022
@glennsarti glennsarti deleted the gs/add-run-tasks branch June 14, 2022 01:57
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.

None yet

3 participants