Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Prevent modifying and/or deleting reserved statuses #6817

Merged
ocket8888 merged 7 commits into from
May 13, 2022
Merged

Prevent modifying and/or deleting reserved statuses #6817

ocket8888 merged 7 commits into from
May 13, 2022

Conversation

@srijeet0406
Copy link
Contributor

@srijeet0406 srijeet0406 commented May 9, 2022 •
edited
Loading

This PR closes #6291

Which Traffic Control components are affected by this PR?

  • Traffic Ops

What is the best way to verify this PR?

Run TO and TP locally.
Try to modify/ delete one of the following statuses. You should get an error while doing so.
Reserved status names:
PRE_PROD, ONLINE, OFFLINE, ADMIN_DOWN, REPORTED

If this is a bugfix, which Traffic Control versions contained the bug?

  • master

PR submission checklist

@ocket8888 ocket8888 self-assigned this May 10, 2022
@ocket8888 ocket8888 added bug something isn't working as intended Traffic Ops related to Traffic Ops medium impact impacts a significant portion of a CDN, or has the potential to do so labels May 10, 2022
@mitchell852
Copy link
Member

mitchell852 commented May 10, 2022 •
edited
Loading

just curious. is "PRE_PROD" a reserved status? i.e. is there code that looks specifically for PRE_PROD and performs certain actions based on that status? is this status seeded? i was kind of under the impression Comcast added this status to support an internal process.

@srijeet0406
Copy link
Contributor Author

srijeet0406 commented May 10, 2022

just curious. is "PRE_PROD" a reserved status? i.e. is there code that looks specifically for PRE_PROD and performs certain actions based on that status? is this status seeded? i was kind of under the impression Comcast added this status to support an internal process.

I added it in there for two reasons:

  1. The issue mentions it.
  2. Our seeds file adds this status as part of the seeding process, so I assumed it was reserved.
    That said, I can remove it if we think that it shouldn't be reserved. Not sure if/ when it got to be a reserved status.

ocket8888
ocket8888 reviewed May 10, 2022
View reviewed changes
@ocket8888
Copy link
Contributor

ocket8888 commented May 10, 2022 •
edited
Loading

just curious. is "PRE_PROD" a reserved status? i.e. is there code that looks specifically for PRE_PROD and performs certain actions based on that status? is this status seeded? i was kind of under the impression Comcast added this status to support an internal process.

Comcast may have done that, but it's seeded into everyone's TODB on installation. There is no code in ATC that checks for and acts explicitly on that status. It has no non-semantic meaning.

@mitchell852
Copy link
Member

mitchell852 commented May 10, 2022

Comcast may have done that, but it's seeded into everyone's TODB on installation

ok well is guess since it's "seeded" it should be immutable

ocket8888
ocket8888 reviewed May 11, 2022
View reviewed changes
Copy link
Contributor

@ocket8888 ocket8888 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The changes to the tests are basically the same in all four of the places you're making them, so I just put the comment I had on v4 to avoid repeating myself a bunch of times.

Also, though, now that any cache status that t3c could possibly care about is guaranteed to always exist, they shouldn't need to have any fixture data or testing functions at all. Can probably even be removed from their TCObj/WithObj stuff.

ocket8888
ocket8888 reviewed May 12, 2022
View reviewed changes
@ocket8888 ocket8888 merged commit aa24989 into apache:master May 13, 2022
@asf-ci asf-ci mentioned this pull request Jun 1, 2022
Merged
4 tasks
zrhoffman pushed a commit to zrhoffman/trafficcontrol that referenced this pull request Oct 2, 2022
@srijeet0406
Prevent modifying and/or deleting reserved statuses (apache#6817)
45d43dd 
* prevent modifying and/or deleting reserved statuses

* fix tests

* cleanup

* code review fixes

* fix test

* code review round 2

* remove unused file
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@ocket8888 ocket8888 ocket8888 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ocket8888 ocket8888

Labels

bug something isn't working as intended medium impact impacts a significant portion of a CDN, or has the potential to do so Traffic Ops related to Traffic Ops

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Important Statuses can be deleted and modified

3 participants

@srijeet0406 @mitchell852 @ocket8888