FIX #28, Fix #29 #31
FIX #28, Fix #29 #31
Conversation
- Deprecated Import#Subject and Import#To, replaced by Import#RemoteSubject and Import#LocalSubject respectively. - On read, Import#Subject is mapped to Import#RemoteSubject and Import#Subject is cleared - On read, Import#To is mapped to Import#LocalSubject and Import#To is cleared - Added Migrated() to claim interface to allow toolchains understand if a JWT was migrated, all code must be updated to refer only to RemoteSubject and LocalSubject. Fix #30 - Removed restriction where services cannot have wildcards. Of course they can. Fixed linter warnings about errors and url collisions.
aricart
requested review from
derekcollison and
sasbury
and removed request for
derekcollison
|
This is a breaking change - and after merge the library needs to bump to 2.0.0 to maintain SEM. |
| if i.RemoteSubject != "" || i.LocalSubject != "" { | ||
| return nil, fmt.Errorf("import [%s] uses subject/to and remote/local - only remote/local or subject/to allowed for reading", i.Name) | ||
| } | ||
| i.RemoteSubject = i.Subject |
There was a problem hiding this comment.
the meaning of subject and to depends on the type of the import. We need to do the flipflop here so that we don't have to do it anywhere else.
| @@ -53,6 +53,7 @@ type Claims interface { | |||
| Claims() *ClaimsData | |||
| Encode(kp nkeys.KeyPair) (string, error) | |||
| ExpectedPrefixes() []nkeys.PrefixByte | |||
| Migrated() bool | |||
There was a problem hiding this comment.
How is this used. I see it is now in all the claims, but i don't see where we use it. Do we need this?
|
|
||
| vr := CreateValidationResults() | ||
| i.Validate("", vr) | ||
|
|
||
| if len(vr.Issues) != 2 { | ||
| t.Errorf("imports without token or url should warn the caller, as should wildcard service") | ||
| if len(vr.Issues) != 1 { |
There was a problem hiding this comment.
why did the results of validation change?
|
|
||
| imports := &Imports{} | ||
| imports.Add(i, i2) | ||
|
|
||
| vr := CreateValidationResults() | ||
| imports.Validate("", vr) | ||
|
|
||
| if len(vr.Issues) != 3 { | ||
| t.Errorf("imports without token or url should warn the caller x2, wildcard service as well") | ||
| if len(vr.Issues) != 2 { |
There was a problem hiding this comment.
why did this change - did you change wildcard service? Derek wants to save that for later i think
|
We chatted about it for a bit, because if we look carefully at the different applications and we still have to make the to/subject flips - unless we are willing to normalize and remove the flips there's no point in having the deprecated fields live forever. |
Fix #28
Fix #30 - Removed restriction where services cannot have wildcards. Of course, they can.
Fixed linter warnings about errors and
urlpackage collisions.