Added deprecated domain check to the taskvalidator #5580
Conversation
agautam478
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
| {"DomainFetchSuccess", "domain-id-success", "run-id-success"}, | ||
| {"DomainFetchFailure", "domain-id-failure", "run-id-failure"}, |
There was a problem hiding this comment.
how are these 2 cases different? validations expect no error below
There was a problem hiding this comment.
The main code currently doesn't do anything except for logging the case. The test is for a supporting domain id retrieval check as there's not much to test really. Will be modifying it with something more substantial in the upcoming in-progress PR.
| const ( | ||
| DeprecatedDomainStatus = 1 | ||
| DeletedDomainStatus = 2 | ||
| ) |
There was a problem hiding this comment.
these are still copies of original enum. see my prev comment #5580 (comment)
There was a problem hiding this comment.
@Shaddoll what is the pattern we follow for defining a corresponding type in common that is originally defined in persistence package only?
There was a problem hiding this comment.
I tried importing the original enum but it didn't work out so ended up defining these in a common file.
There was a problem hiding this comment.
they're visible / common/persistence is definitely the place to get them from when looking at persistence-related data. should work the same since they're consts (i.e. same implicit casting whether it's here or persistence folder)
There was a problem hiding this comment.
@agautam478 why didn't it work when imported from persistence? cyclic dependency?
We need to figure out a solution than re-defining these because there's no mechanism ensuring these don't go out of sync
What changed?
Why?
How did you test it?
Added a partner unit test.
Potential risks
NA
Release notes
NA
Documentation Changes
NA