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.

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

feat(resource-status): Return PAUSED status for vetoed resources #572

Merged
merged 8 commits into from
Nov 6, 2019

Conversation

luispollo
Copy link
Contributor

@luispollo luispollo commented Nov 1, 2019

Closes #561

After discussing with @emjburns, I implemented an override of the resource status in ApplicationController, which is what the UI calls to get resource summaries, so that it was possible to use the auto-wired VetoEnforcer bean without introducing new cross-module dependencies (the ResourceRepository interface, which is where status is currently calculated for resources, is in keel-core, and VetoEnforcer is in keel-veto). This works, but has the undesired side-effect that the PAUSED status as defined by a presence of any veto on a resource is only visible from the /application API. For example, if you call /resources you'll still get whatever the "real" status of the resource is.

@asher @robfletcher If you have any ideas on how to do this in a different/better way, I'd be happy to change things around.

@erikmunson FYI.

Update: based on feedback below, I revisited the implementation to follow the existing pattern of emitting ResourceEvents from ResourceActuator, and introduced new events for when actuation is paused/resumed. I kept the tests for ApplicationController (which are now unrelated) because we didn't have any before. I extracted the logic to parse the app name from a ResourceId into a getter so it can be used consistently. Hope this will make more sense now.

P.S. I think I may have uncovered an issue with different assumptions we make about the ResourceId format in different places (see FIXME comment in ApplicationVeto).

@emjburns
Copy link
Contributor

emjburns commented Nov 1, 2019

@luispollo we could add the same sort of logic into the ResourceController to check if a resource has been "paused", and apply the same override. But, I'm not sure if that's the best way.

Re: ResourceId. Yes, we don't have a format for it that's defined. Are you seeing any problems currently, or are you just wary that we have to be careful not to create problems for ourselves?

I think we chose to have a semi-parseable string given the vague feeling of "it's easy to look at, serialize, we've done things for redis like this before, and right now there aren't many resource types". I agree that it makes working with the IDs a little more challenging though.

@robfletcher
Copy link
Contributor

Why don't we just trigger a new ResourceEvent type when we pause management or resume management?

@emjburns
Copy link
Contributor

emjburns commented Nov 1, 2019

@robfletcher that's a great idea! That way, the status of the resource will be reflected easily.

@erikmunson
Copy link
Member

Oh hey I love that idea — then you could actually follow the chain of events in the history view too. Event names could be something like ResourceActuationPaused and ResourceActuationResumed?

@luispollo
Copy link
Contributor Author

@gal-yardeni FYI

*
* @param id the resource id.
*/
fun lastEvent(id: ResourceId): ResourceEvent? = eventHistory(id, 1).firstOrNull()
Copy link
Contributor

Choose a reason for hiding this comment

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

Super useful 👍

@emjburns emjburns added the ready to merge Approved and ready for merge label Nov 6, 2019
@mergify mergify bot merged commit f9fec5c into spinnaker:master Nov 6, 2019
@mergify mergify bot added the auto merged label Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged ready to merge Approved and ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apply PAUSED status to resources which a veto currently applies to
4 participants