-
Notifications
You must be signed in to change notification settings - Fork 3
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
Support ChirpStack v4 #112
Conversation
5e4223a
to
6a6c1f7
Compare
## [v0.11.2] (2024-03-04) | ||
|
||
### Fixed | ||
|
||
- Exporting devices from ChirpStack v3. | ||
|
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.
This is a mistake in the current (released) changelog that's fixed.
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.
LGTM, is this tested for OTAA and ABP?
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.
LGTM
c.JoinEUI = &types.EUI64{} | ||
if err := c.JoinEUI.UnmarshalText([]byte(c.joinEUI)); err != nil { | ||
return errInvalidJoinEUI.WithAttributes("join_eui", c.joinEUI) | ||
} | ||
c.caCertPath = os.Getenv("CHIRPSTACK_CA_CERT_PATH") |
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.
Won't this always override the CLI flags ?
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.
Yes indeed. We do this for all the other sources the same currently. I think we should fix this separately for all the sources.
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.
I would like to see this fixed everywhere before we make a new release, as it is very broken.
The change itself comes from #97 (comment) , but the idea is that only secrets shouldn't be initialized as flag defaults - the rest of the setup does not need this.
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.
Sounds good to me. I'll file a follow up before we make a release.
Summary
Closes #58 and replaces #75
Changes
Testing
Locally
Regressions
NA. This is supporting a new API version.
Notes for Reviewers
This removes support for CS v3. This PR will target
v0.12.0
and subsequent versions will only support CS v4.CS v3 will remain supported in
v0.11.x
and Fix errors and panics in the chirpstack v3 source #111 will make sure that v3 works.This is blocked on Fix errors and panics in the chirpstack v3 source #111 so that it can be released first.
TODO: Update documentation for CSv3 and CSv4 Add documentation for ChirpStack v4 TheThingsIndustries/lorawan-stack-docs#1281
Checklist
CHANGELOG.md
.