search

Reviewable / appengine-remove-action

Action to remove versions of deployed App Engine apps over the set limit
3 stars 2 forks source link

Added Apply Limit After Days option #6

Closed ben-wckdrzr closed 2 years ago

ben-wckdrzr commented 2 years ago

This seems like the best way to implement this option, will leave all versions within the days-to-keep range, and keep the limited number of versions from before that.

Let me know if you think it should be different.

All tested and working for me.

This change is Reviewable

pkaminski commented 2 years ago

Thanks -- I'll take a look in a few days when I get back home.

ben-wckdrzr commented 2 years ago

Hi, thanks for the review

I’m not sure I agree having the limit with the days_to_keep is right, although it probably does read better.

I see the main use-case for this is when I have a flurry of changes on one day (more than the limit), but then decide I wan’t to quickly get back to yesterday, or earlier. In this use case we are likely exempting all versions beyond the limit.

That said, I feel like I wanted it as you say, until I realised this implementation was significantly simpler, although I’m now not convinced of the benefit of a more complex approach…

To my mind, this is a good first version approach, that could be build upon in a later version.

Potentially a better name than ‘days_to_keep’ would make all the above clearer??

Ben

ben-wckdrzr commented 2 years ago

Sounds good - updated and tested 👍

pkaminski commented 2 years ago

v2.2.0 is out now -- sorry it took a while.