Various event-related fixes and improvements #2565
Conversation
rvolosatovs
requested review from
adriansmares,
bafonins,
johanstokking,
KrishnaIyer,
kschiffer and
pgalic96
as code owners
rvolosatovs
force-pushed
the
feature/various-event-related
branch
from
6ff505d
to
24233f4
Compare
rvolosatovs
removed request for
htdvisser,
kschiffer,
KrishnaIyer,
bafonins,
pgalic96 and
adriansmares
There was a problem hiding this comment.
Passes sanity check, but needs @htdvisser approval as he may have had some ideas around event start/stop and API behavior
| if len(req.Identifiers) == 0 { | ||
| return nil | ||
| return errNoIdentifiers |
There was a problem hiding this comment.
This is an API behavioral change
There was a problem hiding this comment.
Doesn't this fall under these two:
- We will not break the API between components and events within minor versions. So at least the same minor versions of components are compatible with each other.
- We reserve the right to fix bugs in API, configuration and storage in patches and minor updates. This may break components, gateways and applications that rely on buggy behavior.
IMO silently failing here is a bug
There was a problem hiding this comment.
I don't see a bug report that is referenced
| @@ -45,7 +45,6 @@ func unmarshalContext(ctx context.Context, data map[string][]byte) (context.Cont | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| delete(data, name) | |||
There was a problem hiding this comment.
This change makes that unmarshalContext and marshalContext are no longer symmetric. Not sure if that was intended.
| for _, entityIDs := range evt.Identifiers() { | ||
| // EventIsVisible returns whether ev is visible given rights in the context. | ||
| func EventIsVisible(ctx context.Context, ev events.Event) (bool, error) { | ||
| visibility := ev.Visibility().Implied() |
There was a problem hiding this comment.
This is not correct in this context.
Maybe Implied() is a bit confusing. If a user has the "admin right", that implies that the user has all rights. If an event is visible to callers with the "admin right", that doesn't mean it's also visible to the implicit rights (i.e. for a caller with only the "info right").
rvolosatovs
force-pushed
the
feature/various-event-related
branch
2 times, most recently
from
67bc32f
to
9c2e8a1
Compare
rvolosatovs
force-pushed
the
feature/various-event-related
branch
from
9c2e8a1
to
c814274
Compare
Summary
References https://github.com/TheThingsIndustries/lorawan-stack/pull/2096
Close #2569
Changes
rightsutil.EventIsVisibletest.AllTrueand use in NSChecklist
README.md. The target branch is set tomasterif the changes are fully compatible with existing API, database, configuration and CLI.CHANGELOG.md.CONTRIBUTING.md, there are no fixup commits left.