Skip to content
This repository has been archived by the owner on Oct 17, 2024. It is now read-only.

API simplification #118

Merged
merged 48 commits into from
May 22, 2019
Merged

API simplification #118

merged 48 commits into from
May 22, 2019

Conversation

asdine
Copy link
Contributor

@asdine asdine commented May 21, 2019

Package regula: Removed redundant ruleset validation code, the api package already does that

Package errors: Removed unnecessary errors, these errors should not be "catchable" and represent something bad that shouldn't happen in normal conditions

Package api:
List now returns a simple list of paths, with pagination. No prefix.
• Put now returns the version string, not the ruleset
• Renamed ContinueToken to Cursor

Package api/etcd:
• The change of behaviour of the List and Put APIs simplified a lot the etcd implementation. Lots of code removed
• Tests have been split into multiple files

Package http:
• Tests have been rewritten into table tests and split
• Applied changes from the API

⚠️ Tests are failing on purpose, next PRs will fix all of that

@asdine asdine added the WIP label May 21, 2019
@asdine asdine requested review from tealeg and yaziine May 21, 2019 12:35
@asdine asdine removed the WIP label May 21, 2019
Copy link
Contributor

@tealeg tealeg left a comment

Choose a reason for hiding this comment

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

Really good work, just a few small things.

api/etcd/get.go Outdated Show resolved Hide resolved
if version == "" {
_, version = s.pathVersionFromKey(string(resp.Responses[2].GetResponseRange().Kvs[0].Key))
version = versions[len(versions)-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

We could also have stored the result of the len we did on the kvs above. Micro-optimisation, mind you ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, IIRC len is constant time, unlike C's strlen which is O(n) and justifies caching the length to reuse it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not just constant time - it can even be registerised if the compiler decides it's worth it :)

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I knew it was constant time, but pushing it to the register, I didn't.

api/etcd/get_test.go Show resolved Hide resolved
api/etcd/list_test.go Outdated Show resolved Hide resolved
api/service.go Outdated Show resolved Hide resolved
Copy link

@yaziine yaziine left a comment

Choose a reason for hiding this comment

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

Good job man! 👍

Few comments to address.

api/etcd/eval_test.go Outdated Show resolved Hide resolved
api/etcd/list.go Show resolved Hide resolved
api/etcd/list.go Outdated Show resolved Hide resolved
api/etcd/get_test.go Outdated Show resolved Hide resolved
@asdine asdine requested a review from tealeg May 22, 2019 10:09
@asdine
Copy link
Contributor Author

asdine commented May 22, 2019

🆙 @tealeg @Yassinee

Copy link
Contributor

@rogpeppe rogpeppe left a comment

Choose a reason for hiding this comment

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

LGTM FWIW; nice simplifications. I haven't looked at the test code in detail, but I've got a few minor comments and suggestions.

api/etcd/get.go Show resolved Hide resolved
if version == "" {
_, version = s.pathVersionFromKey(string(resp.Responses[2].GetResponseRange().Kvs[0].Key))
version = versions[len(versions)-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Not just constant time - it can even be registerised if the compiler decides it's worth it :)

api/etcd/get.go Outdated Show resolved Hide resolved
api/service.go Outdated
PathsOnly bool // return only the paths of the rulesets
AllVersions bool // return all versions of each rulesets
Limit int // If the Limit is lower or equal to 0 or greater than 100, it will be set to 50 by default.
Cursor string // pagination cursor
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps mention that if this is empty, the list starts from the beginning?

api/service.go Show resolved Hide resolved
api/etcd/list.go Show resolved Hide resolved
@asdine asdine merged commit 6456559 into signature-api May 22, 2019
@asdine asdine deleted the rework-list-api branch May 22, 2019 14:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants