Skip to content

usage_limit: remove seconds from periods set#21

Merged
bors[bot] merged 2 commits intomasterfrom
remove-seconds-from-periods-set
Apr 27, 2018
Merged

usage_limit: remove seconds from periods set#21
bors[bot] merged 2 commits intomasterfrom
remove-seconds-from-periods-set

Conversation

@eguzki
Copy link
Copy Markdown
Member

@eguzki eguzki commented Apr 23, 2018

"seconds" level of granularity does not exist for usage limits. It was tried to load and never found.

Comment thread lib/3scale/backend/usage_limit.rb Outdated
include Storable

PERIODS = Period::ALL_DESC
PERIODS = Period::ALL_DESC.reject { |period| period == :second }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

.freeze?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Another option:

Period::ALL_DESC - [:second]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This

Period::ALL_DESC - [:second]

does not work. It is an array of granularity class objects created here

But somehow (not investigated yet why)

Period::ALL_DESC.reject { |period| period == :second }

works

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Because the equality is overridden - set operations are probably looking for an implementation of hash which differs between different objects, so they are not seen as equal. Using Period::Second or Period.from :second would likely work.

@davidor
Copy link
Copy Markdown
Contributor

davidor commented Apr 23, 2018

I think we should add tests for this.

@unleashed unleashed mentioned this pull request Apr 25, 2018
@unleashed
Copy link
Copy Markdown
Contributor

@eguzki at minimum we need the constant frozen to merge this

@eguzki eguzki force-pushed the remove-seconds-from-periods-set branch 3 times, most recently from dd3a757 to 2c6e8b0 Compare April 27, 2018 11:29
@eguzki
Copy link
Copy Markdown
Member Author

eguzki commented Apr 27, 2018

unittests added

@eguzki eguzki force-pushed the remove-seconds-from-periods-set branch from 2c6e8b0 to 7f1bb5f Compare April 27, 2018 11:36
Copy link
Copy Markdown
Contributor

@unleashed unleashed left a comment

Choose a reason for hiding this comment

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

LGTM

@davidor
Copy link
Copy Markdown
Contributor

davidor commented Apr 27, 2018

bors r=@unleashed,@davidor

@unleashed
Copy link
Copy Markdown
Contributor

@davidor perhaps the edit was not caught by bors?

bors r=@unleashed,@davidor

bors Bot added a commit that referenced this pull request Apr 27, 2018
21: usage_limit: remove seconds from periods set r=unleashed,davidor a=eguzki

"seconds" level of granularity does not exist for usage limits. It was tried to load and never found.

Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
@bors
Copy link
Copy Markdown
Contributor

bors Bot commented Apr 27, 2018

Build failed

@unleashed
Copy link
Copy Markdown
Contributor

heisentest again :/

bors r=@unleashed,@davidor

bors Bot added a commit that referenced this pull request Apr 27, 2018
21: usage_limit: remove seconds from periods set r=unleashed,davidor a=eguzki

"seconds" level of granularity does not exist for usage limits. It was tried to load and never found.

Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
@bors
Copy link
Copy Markdown
Contributor

bors Bot commented Apr 27, 2018

Build succeeded

@bors bors Bot merged commit 7f1bb5f into master Apr 27, 2018
@bors bors Bot deleted the remove-seconds-from-periods-set branch April 27, 2018 14:42
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