Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set location metadata in uplink messages forwarded by the AS #3294

Merged
merged 4 commits into from
Oct 8, 2020

Conversation

neoaggelos
Copy link
Contributor

@neoaggelos neoaggelos commented Sep 29, 2020

Summary

Closes #2669

Changes

  • Add locations field in ApplicationUplink messages.
  • Set field when handling uplink message in the AS.

Testing

  • Tested locally, working on unit tests.

Notes for Reviewers

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

Sorry, something went wrong.

@neoaggelos neoaggelos added c/application server This is related to the Application Server blocked This can't continue until another issue or pull request is done labels Sep 29, 2020
@neoaggelos neoaggelos added this to the September 2020 milestone Sep 29, 2020
@neoaggelos neoaggelos self-assigned this Sep 29, 2020
@github-actions github-actions bot added c/identity server This is related to the Identity Server compat/api This could affect API compatibility compat/config This could affect Configuration compatibility documentation This involves writing user documentation labels Sep 29, 2020
@neoaggelos
Copy link
Contributor Author

Blocked by #3293

Copy link
Contributor

@adriansmares adriansmares left a comment

Choose a reason for hiding this comment

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

Target your cache PR in order to distinguish the changes.

api/messages.proto Outdated Show resolved Hide resolved
@htdvisser htdvisser modified the milestones: September 2020, October 2020 Oct 1, 2020
@johanstokking
Copy link
Member

Lots of reviewers here; @neoaggelos who should do what?

@neoaggelos
Copy link
Contributor Author

Lots of reviewers here; @neoaggelos who should do what?

This is because of CODEOWNERS. This PR is currently blocked waiting for #3293

Copy link
Member

@johanstokking johanstokking left a comment

Choose a reason for hiding this comment

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

I'm not sure if I like the high coupling in AS this way. For inspiration, please see the tenant fetcher which has a cache.

You might want to change the cache interface to a generic "getter" with callback to fetch the entity if needed. The concrete implementation can then always use the callback to get the entity (no cache), use a memory cache, etc.

cmd/ttn-lw-stack/commands/start.go Outdated Show resolved Hide resolved
cmd/ttn-lw-stack/commands/start.go Outdated Show resolved Hide resolved
pkg/applicationserver/device_cache.go Outdated Show resolved Hide resolved
pkg/applicationserver/applicationserver.go Outdated Show resolved Hide resolved
pkg/applicationserver/applicationserver.go Outdated Show resolved Hide resolved
@johanstokking
Copy link
Member

Wait, is #3293 the better version of this?

@neoaggelos
Copy link
Contributor Author

Wait, is #3293 the better version of this?

#3294 (comment). #3293 is the device fetcher extracted as a separate PR, so that this can focus on the proto changes instead.

@adriansmares
Copy link
Contributor

adriansmares commented Oct 2, 2020

Wait, is #3293 the better version of this?

#3294 (comment). #3293 is the device fetcher extracted as a separate PR, so that this can focus on the proto changes instead.

If you make such chains of PRs, please actually make them target one another instead of making it look like this PR introduces changes that it doesn't. While you indeed added the blocking/blocks labels and comments, the boundaries between the PRs are unclear and as such the reviewers do not actually know which change in the code belongs which.

If you would have targeted your branch from #3293 here, instead of 3.10, only the API protos changes would have been visible (exactly what you wanted).

GitHub would have also automatically changed the target of this branch back to 3.10 once #3293 was merged, so you don't end up with lingering references.

@neoaggelos neoaggelos force-pushed the feature/as-messages-location-metadata branch from 6df9e8e to 823eb1a Compare October 2, 2020 13:52
@neoaggelos neoaggelos force-pushed the feature/as-messages-location-metadata branch from 823eb1a to 9d44c8a Compare October 2, 2020 14:10
@neoaggelos neoaggelos requested review from johanstokking and htdvisser and removed request for kschiffer and bafonins October 2, 2020 14:30
pkg/applicationserver/applicationserver.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked This can't continue until another issue or pull request is done c/application server This is related to the Application Server c/identity server This is related to the Identity Server compat/api This could affect API compatibility compat/config This could affect Configuration compatibility documentation This involves writing user documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Device location not included in metadata
4 participants