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

Extend the TaskEngine interface with the GetTaskByArn method #340

Merged
merged 2 commits into from
Mar 15, 2016

Conversation

aaithal
Copy link
Contributor

@aaithal aaithal commented Mar 14, 2016

@samuelkarp
Copy link
Contributor

agent/stats/engine_test.go:314: cannot use taskEngine (type *MockTaskEngine) as type engine.TaskEngine in argument to statsEngine.MustInit:

    *MockTaskEngine does not implement engine.TaskEngine (missing GetTaskByArn method)

Can you regenerate the mocks?

@aaithal
Copy link
Contributor Author

aaithal commented Mar 14, 2016

Yeah, noticed that. Regenerating mocks now.

@aaithal
Copy link
Contributor Author

aaithal commented Mar 14, 2016

Updated PR now passes tests. r? @samuelkarp @juanrhenals

@samuelkarp
Copy link
Contributor

👍

@@ -54,6 +54,10 @@ func (engine *MockTaskEngine) ListTasks() ([]*api.Task, error) {
return nil, nil
}

func (engine *MockTaskEngine) GetTaskByArn(arn string) (*api.Task, bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: is there any particular reason why you're returning a bool type here instead of just using error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, just following the map lookup semantics in golang.

@juanrhenals
Copy link
Contributor

Just had one nit. LGTM 👍

aaithal added a commit that referenced this pull request Mar 15, 2016
Extend the TaskEngine interface with the GetTaskByArn method
@aaithal aaithal merged commit c6b4bcc into aws:dev Mar 15, 2016
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.

3 participants