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

Use Task group to filter on list tasks instead of StartedBy #258

Merged
merged 3 commits into from
Jun 14, 2017

Conversation

yinshiua
Copy link
Contributor

closes #97

Testing

$ make build

$ make test

Tasks:

$ ecs-cli compose up

$ ecs-cli compose ps
// Shows the correct task IDs (does not include the ones started by service)

Service:

$ ecs-cli compose service up

$ ecs-cli compose service ps
// Shows the correct task IDs

@yinshiua yinshiua added the bug label Jun 12, 2017
@yinshiua yinshiua requested a review from aaithal June 12, 2017 23:18
Copy link

@aaithal aaithal left a comment

Choose a reason for hiding this comment

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

Looks mostly good. I have a small comment. Can you also confirm that Tasks that have already been started using the "started by" field continue to show up with this change?

@@ -120,7 +120,18 @@ func CollectTasksWithStatus(entity ProjectEntity, status string, filterLocal boo
result := []*ecs.Task{}

err := entity.Context().ECSClient.GetTasksPages(request, func(respTasks []*ecs.Task) error {
result = append(result, respTasks...)
// Filter the results by task.Group
if entity.EntityType() == "task" && filterLocal {
Copy link

Choose a reason for hiding this comment

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

  1. Can you use a const for comparison instead of "task"? Also, it'd make more sense for EntityType() to return an enum instead of a string (https://github.com/aws/amazon-ecs-cli/blob/master/ecs-cli/modules/cli/compose/entity/entity.go#L42)

  2. Can you update the comment for CollectTasksWithStatus to reflect this change as well? Right now, filterLocal is only mentioned in the context of startedBy

// express or implied. See the License for the specific language governing
// permissions and limitations under the License.

package entityType
Copy link

Choose a reason for hiding this comment

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

this can just be type. enitity.entityType stutters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

type is used in go, don't think I can name it as type, but I can name it as types. What do you think? Right now is entityType.Type, but with the change it'll be types.EntityType

Copy link

Choose a reason for hiding this comment

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

types is fine. You can just name it types.Type ?

@yinshiua
Copy link
Contributor Author

yes, I've tested using running ecs-cli compose up with the old cli and run ecs-cli compose ps with the change.

@yinshiua yinshiua merged commit 8db5d31 into aws:master Jun 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error listing tasks, InvalidParameterException: ownerId longer than 36
2 participants