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.

Sign up for GitHub

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

Jump to bottom

feat(rule): Adding a general tag rule #233

Merged
merged 8 commits into from
Oct 29, 2019
Merged

feat(rule): Adding a general tag rule #233

merged 8 commits into from
Oct 29, 2019

Conversation

jeyrschabu
Copy link
Contributor

@jeyrschabu jeyrschabu commented Jul 23, 2019 •
edited
Loading

  • added ExpiredResourceRule to apply to resources that are tagged with a ttl.
  • rule evaluates if resource has expired.

@aravindmd
Copy link
Contributor

Nice ! , do you think adding a @conditional or something else like a kill switch to safeguard this would make sense ?

emjburns
emjburns approved these changes Jul 25, 2019
View reviewed changes
Copy link
Contributor

@emjburns emjburns left a comment

Choose a reason for hiding this comment

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

Overall this looks great to me. My one request always is to include more verbose class comments that explain how to use something or give an example (I've added one comment in the specific area on this PR).

For things that are exposed via config, I find it super helpful to have an example in the class comments for when I'm trying to easily understand how to enable a feature. I think this is arguably worse than actual documentation, but it is easy and more likely to get changed as functionality changes 😆

@aravindmd
Copy link
Contributor

aravindmd commented Jul 25, 2019 •
edited
Loading

Overall this looks great to me. My one request always is to include more verbose class comments that explain how to use something or give an example (I've added one comment in the specific area on this PR).

For things that are exposed via config, I find it super helpful to have an example in the class comments for when I'm trying to easily understand how to enable a feature. I think this is arguably worse than actual documentation, but it is easy and more likely to get changed as functionality changes 😆

  • 1 , the comments of test controller are really helpful.

@jeyrschabu
Copy link
Contributor Author

Keeping it simple. BasicRule was a bit over engineered, so removing that for now and keeping the rule to apply when a resource has expired. The expiration time is determined by inspecting the tags on the resource.

@jeyrschabu
feat(rule): Adding a general tag rule
597d08b 
- will match defined tag values in config to the ones on the resource
- if matched and calculated age is older than today, the resource is marked
@jeyrschabu
- A bit of needed cleanup w/r to tags
ee37908 
- make tags a distinct object on the resource for convenience
- temporal tags are managed internally
- fix a bug on ttl calculation
@jeyrschabu
- keep it simple, remove the need for a basic rule
5c59b49 
- renamed TemporalThresholdRule to ExpiredResourceRule
- Updated to use a clock to determine if resource is expired
- clean up test
@jeyrschabu
- s/prefix/suffix/ in docs
09cb36b
@jeyrschabu
- cleanup
9234dbd
@jeyrschabu
- fix amazonTagExclusion based on testing
e506ffd
@jeyrschabu jeyrschabu added the ready to merge Reviewed and ready for merge label Oct 29, 2019
@mergify mergify bot added the auto merged label Oct 29, 2019
@mergify mergify bot merged commit 70f12bd into spinnaker:master Oct 29, 2019
@aravindmd aravindmd mentioned this pull request Nov 7, 2019
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emjburns emjburns emjburns approved these changes

@aravindmd aravindmd aravindmd approved these changes

Assignees
No one assigned
Labels
auto merged ready to merge Reviewed and ready for merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jeyrschabu @aravindmd @emjburns