Enables the use of offline reason for all "offline" statuses (those that are not ONLINE/REPORTED) and makes it optional #7042
Conversation
mitchell852
added
Traffic Ops
| @@ -121,7 +121,7 @@ func UpdateStatusHandler(w http.ResponseWriter, r *http.Request) { | |||
| return | |||
| } | |||
|
|
|||
| if *status.Name == tc.CacheStatusAdminDown.String() || *status.Name == tc.CacheStatusOffline.String() { | |||
| if *status.Name != tc.CacheStatusOnline.String() && *status.Name != tc.CacheStatusReported.String() { | |||
| if reqObj.OfflineReason == nil { | |||
There was a problem hiding this comment.
this actually makes offline reason required for a bunch of statuses (anything that is not online/reported) so if this is considered a breaking api change, i will just hold off until api 5.x....or, the offline reason requirement could simply be removed and always optional if not online/reported.
mitchell852
added
the
improvement
mitchell852
changed the title
| @@ -121,7 +121,7 @@ func UpdateStatusHandler(w http.ResponseWriter, r *http.Request) { | |||
| return | |||
| } | |||
|
|
|||
| if *status.Name == tc.CacheStatusAdminDown.String() || *status.Name == tc.CacheStatusOffline.String() { | |||
| if *status.Name != tc.CacheStatusOnline.String() && *status.Name != tc.CacheStatusReported.String() { | |||
There was a problem hiding this comment.
So if I make a server have the status ONLINE1 or ONLINE_BUT_NOT_IN_SERVICE I then have to enter a reason why my ostensibly online server is offline?
There was a problem hiding this comment.
yes, unless we made "offline reason" always optional (never required as it is now) which makes more sense to me anyhow
There was a problem hiding this comment.
ONLINE1 or ONLINE_BUT_NOT_IN_SERVICE is not considered "online" from TC's perspective :)
There was a problem hiding this comment.
well i guess it could be for some servers :) i.e. TP or TO where status has no impact and is purely informational
There was a problem hiding this comment.
Server statuses should not be mutable. It's one of a million places where we hijacked a concept to provide arbitrary informative tagging, which is better provided by actual tags. As it is, there's no guarantee that a status with any given name exists, or that a status with any given ID won't be renamed arbitrarily at any moment. Doing these kinds of checks becomes a lot easier once we can say what the actual values are and should mean.
There was a problem hiding this comment.
Yeah, i get it but that ship has sailed (the immutable part), we already have many "offline" status types in production (i.e. TEST, PRE_PROD, DECOM, etc) that are all "offline" yet you can't add an offline reason to them.
There was a problem hiding this comment.
That can be retconned:
- Implement tags
- Migrate existing, unrecognized statuses to tags
- Apply those tags on the servers that use those statuses
- Convert all unknown statuses to "OFFLINE"
- Eliminate the concept of a "Status" as an ATC object, use constant strings only.
mitchell852
changed the title
This change treats any status that is not ONLINE/REPORTED as "offline" and therefore the offline reason is available for use.
Which Traffic Control components are affected by this PR?
What is the best way to verify this PR?
If this is a bugfix, which Traffic Control versions contained the bug?
PR submission checklist