Skip to content

Conversation

@arschles
Copy link
Member

@arschles arschles commented Apr 21, 2016

Partially addresses #60, #58 and #57
Fixes #44

Still TODO:

  • Implement (*github.com/deis/workflow-manager-api/data/ClusterFromDB).FilterByAge
  • Add unit test for the previous
  • Add unit test for the GET /:apiVersion/clusters/age?... endpoint
  • Document error cases in the filters in api-architecture.md

@arschles
Copy link
Member Author

I'm punting this to beta4

Copy link
Member

Choose a reason for hiding this comment

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

I think the last sentence fragment here is probably superfluous.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in aa8d553

@jackfrancis
Copy link
Member

LGTM, though I'd like to walk through the 4 documented error types, just so I'm 100% on board (I know you spent a lot of thought teasing those out, it'd be nice to catch up with the fruit of your labor).

Copy link
Member

Choose a reason for hiding this comment

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

s/called/caller ?

Copy link
Member

Choose a reason for hiding this comment

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

not needed anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

removed in 8dc8ac9

complete with tests at multiple layers and API documentationoo
Copy link
Member

Choose a reason for hiding this comment

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

nit/not needed for this PR: looks like the only test 'gap' is validating the 400 Bad Request resp scenarios? (Although the data side appears covered in data/cluster_from_db_filter_by_age_test.go)

Copy link
Member Author

Choose a reason for hiding this comment

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

yep that's right. I've added #85 to track the work to validate the error responses

@arschles arschles merged commit f205e13 into deis:master Apr 28, 2016
@arschles arschles deleted the cluster-age branch April 28, 2016 18:16
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.

3 participants