-
Notifications
You must be signed in to change notification settings - Fork 311
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
Refactor ADR API usage #5353
Merged
Merged
Refactor ADR API usage #5353
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
c/network server
This is related to the Network Server
ui/cli
This is related to ttn-lw-cli
labels
Mar 30, 2022
adriansmares
force-pushed
the
feature/5092-adr-impl
branch
3 times, most recently
from
April 1, 2022 14:45
74015c3
to
2d7a48e
Compare
adriansmares
force-pushed
the
feature/5092-adr-impl
branch
4 times, most recently
from
April 1, 2022 16:49
35e6e57
to
2c8af6b
Compare
adriansmares
requested review from
johanstokking,
htdvisser and
pgalic96
as code owners
April 1, 2022 16:51
adriansmares
force-pushed
the
feature/5092-adr-impl
branch
6 times, most recently
from
April 1, 2022 21:00
ca8b970
to
e42259e
Compare
johanstokking
approved these changes
Apr 12, 2022
adriansmares
force-pushed
the
feature/5092-adr-impl
branch
from
April 12, 2022 15:34
9bca024
to
6ec492c
Compare
adriansmares
force-pushed
the
feature/5092-adr-impl
branch
from
April 12, 2022 15:51
6ec492c
to
504c28e
Compare
pgalic96
approved these changes
Apr 12, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CLI part LGTM
5 tasks
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Closes #3820
Closes #5092
References #40
References TheThingsIndustries/protoc-gen-fieldmask#43
Changes
SetFields
callsoneof
fields, as the bottom level fields could contain multipleoneof
optionsmac_settings.adr
is mutually exclusive withuse_adr
/adr_margin
dev.MacState.*Parameters
spam is not adding any value in my opinion, but let me know your thoughtsTesting
Unit testing and CLI testing. Unit tests have been added for the clamping logic, while the CLI has been used extensively for the validation.
Mutual exclusivity of the fields
Disallowing dynamic ADR settings when the band does not allow it
Parameter well definedness
I'll add these CLI only tests as unit tests in a future PR.
Regressions
As part of this PR I've had to fix some field validation which was previously broken - you will see in the code that some extra loop variable aliases are added, since these variables are captured in certain function definitions which are called later. These checks where previously not working, so it is possible that we now reject some input that was previously accepted as valid. Depending on the reports we will need to check if validation is broken or the request is broken.
Notes for Reviewers
@johanstokking main reviewer.
If you would like to test this locally, regenerate the protos using TheThingsIndustries/protoc-gen-fieldmask#43, otherwise you will have a hard time switching between modes (as currently switching the type of
oneof
field in a single roundtrip is not possible).Checklist
README.md
for the chosen target branch.CHANGELOG.md
.CONTRIBUTING.md
, there are no fixup commits left.