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

(TER-183) Add Description for Run Tasks #447

Merged
merged 2 commits into from Jul 15, 2022
Merged

Conversation

glennsarti
Copy link
Contributor

@glennsarti glennsarti commented Jun 29, 2022

Description

Previously Run Tasks was marked as beta in go-tfe. However Run Tasks is now
Generally Available. This commit updates go-tfe to mark Run Tasks as not
Beta.

Run Tasks now have an optional description field. This commit updates
the RunTask API endpoints to include the description where applicable
(Create/Read/Update).

Testing plan

N/A Integration tests updated

External links

Output from tests

N/A Circle CI output

@glennsarti glennsarti changed the title {DO NOT MERGE} Add Description for Run Tasks (TER-183) Add Description for Run Tasks Jun 29, 2022
@glennsarti glennsarti force-pushed the gs/add-task-description branch 2 times, most recently from c748067 to 9f904b8 Compare June 29, 2022 05:46
.circleci/config.yml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
helper_test.go Outdated
t.Errorf("Error destroying workspace! WARNING: Dangling resources\n"+
"may exist! The full error is shown below.\n\n"+
"Workspace: %s\nError: %s", w.Name, err)
if errors.Is(err, ErrResourceNotFound) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain this change?

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 one of the integration tests was failing (checking for removed workspace). It was due to the test cleanup failing; errored trying to remove a workspace that was already removed.

So it begged the question; If you try to cleanup a workspace that's already been cleaned up, should it throw an err?


However this all moot because I've just noticed the test could be setup in a way I didn't have to do this change. But the question is still valid, and perhaps it should be in a different PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed up

Copy link
Contributor

Choose a reason for hiding this comment

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

@glennsarti That's a good question re what to do with a workspace if it's already been cleaned up. I've opened an internal issue for the team to discuss this and update our contributing docs with some guidance.

Copy link
Contributor

@annawinkler annawinkler left a comment

Choose a reason for hiding this comment

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

This is looking good! ⭐ I think we might want to move the CircleCI environment variable to CircleCI (and not in the config.yml) for consistency with the other environment variables used for testing. I had a few other questions that I have left as comments in the PR.

@glennsarti
Copy link
Contributor Author

Thanks @annawinkler , I also got side tracked while doing this PR due to unrelated CI failures so I didn't come back to it :-)

@glennsarti glennsarti force-pushed the gs/add-task-description branch 3 times, most recently from f1fd87d to 3229f93 Compare July 7, 2022 05:27
@glennsarti
Copy link
Contributor Author

@annawinkler Can I get a +1 on this so I can merge.

Previously Run Tasks was marked as beta in go-tfe. However Run Tasks is now
Generally Available. This commit updates go-tfe to mark Run Tasks as not
Beta.
Run Tasks now have an optional description field. This commit updates
the RunTask API endpoints to include the description where applicable
(Create/Read/Update).
@glennsarti
Copy link
Contributor Author

Ping @annawinkler

@@ -73,14 +73,14 @@ const (
type RunTaskListOptions struct {
ListOptions
// Optional: A list of relations to include with a run task. See available resources:
// https://www.terraform.io/cloud-docs/api-docs/run-tasks#list-run-tasks
// https://www.terraform.io/cloud-docs/api-docs/run-tasks/run-tasks#list-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.

🎉

Copy link
Contributor

@Uk1288 Uk1288 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@glennsarti
Copy link
Contributor Author

Thanks @Uk1288

@glennsarti glennsarti dismissed annawinkler’s stale review July 15, 2022 03:24

No longer true and addressed issues raised.

@glennsarti glennsarti merged commit b2dd7ab into main Jul 15, 2022
@glennsarti glennsarti deleted the gs/add-task-description branch July 15, 2022 03:25
@github-actions
Copy link

Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants