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

add /connectivity - endpoint to check the network status of a device #69

Merged
merged 11 commits into from
Nov 22, 2023

Conversation

maxl2287
Copy link
Contributor

What type of PR is this?

Add one of the following kinds:

  • enhancement/feature

What this PR does / why we need it:

This PR creates a new endpoint "/connectivity" to check the current network status of a device.

Which issue(s) this PR fixes:

Fixes #34

@maxl2287 maxl2287 changed the title Draft/check status for device add /connectivity - endpoint to check the network status of a device Aug 15, 2023
@maxl2287 maxl2287 force-pushed the draft/check-status-for-device branch from 2a66f97 to ce07f26 Compare August 16, 2023 07:04
@eric-murray eric-murray marked this pull request as draft August 16, 2023 11:55
@eric-murray
Copy link
Collaborator

This will need to wait until v0.5.0 is published. There are still some issues that need to be resolved for that version.

@maxl2287
Copy link
Contributor Author

Hi @eric-murray,

yes I thought so, too.
That's why I updated the version to 0.6.0-wip.

@eric-murray
Copy link
Collaborator

v0.5.0-wip will need some further updates before it becomes v0.5.0, so let's wait for that to be complete before considering this PR. You can rebase this PR once the final changes to 0.5.0-wip are agreed.

@bigludo7
Copy link
Collaborator

@maxl2287 Agree with @eric-murray that we must first stabilize the v0.5 with the notification and the proceed this PR in v0.6.
Hope we will be able to solve quickly the notification.

One question - Why did you removed CONNECTED_DATA_SMS value ? (the device is connected to the network for Data & SMS usage). Sorry if i missed a discussion on this topic but IMHO it makes sense to keep the value when the device can receive Data &SMS. Thanks

@maxl2287
Copy link
Contributor Author

maxl2287 commented Aug 30, 2023

One question - Why did you removed CONNECTED_DATA_SMS value ? (the device is connected to the network for Data & SMS usage). Sorry if i missed a discussion on this topic but IMHO it makes sense to keep the value when the device can receive Data &SMS. Thanks

@bigludo7
I thought that we can remove it, because it's already inside of CONNECTED_DATA.
Why? - Because I do not know a usecase, where you are connected to data only without be able to receive SMS.
So CONNECTED_DATA_SMS is for me the same like CONNECTED_DATA.

What do you think about this?

@bigludo7
Copy link
Collaborator

Thanks @maxl2287 for the quick answer. Got it. I will check internally with my network experts but makes sense to me now.

@rartych rartych mentioned this pull request Sep 4, 2023
@akoshunyadi
Copy link
Collaborator

Decision from the DeviceStatus working group call: this PR should be extended by the connectivity events. However they should already be based on CloudEvents. To avoid conflicts this PR should be based on PR #74 .

@maxl2287
Copy link
Contributor Author

@eric-murray & @bigludo7:

Depending on the MeetingMinutes (https://github.com/camaraproject/DeviceStatus/blob/main/documentation/MeetingMinutes/MoM-2023-09-13.md)
this PR needs to be part of 0.5.0 right?

I would like to wait until PR #75 is done to do the updates for this PR.

@maxl2287 maxl2287 force-pushed the draft/check-status-for-device branch from ce07f26 to 3806395 Compare November 10, 2023 10:55
@maxl2287 maxl2287 marked this pull request as ready for review November 10, 2023 10:59
@maxl2287
Copy link
Contributor Author

@sachinvodafone @eric-murray @bigludo7
I have updated this PR based on CloudEvents.

Please review. :)

maxl2287 and others added 2 commits November 10, 2023 15:22
Hello @maxl2287 
Thanks for the proposal.
Few cosmetic change and one code change proposal: The eventType org.camaraproject.device-status.v0.subscription-ends has not to be subscribed (see API description). This notification does not require dedicated subscription. But, the eventType could be send in the notification so I've introduced 2 definition of eventType: one with and one without subscription-ends. 

Hope it makes sense for you
Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

Hello @maxl2287
Thanks for the proposal.
Few cosmetic change and one code change proposal: The eventType org.camaraproject.device-status.v0.subscription-ends has not to be subscribed (see API description). This notification does not require dedicated subscription. But, the eventType could be send in the notification so I've introduced 2 definition of eventType: one with and one without subscription-ends.

Hope it makes sense for you

@bigludo7
Copy link
Collaborator

@maxl2287 This is the same comment for camaraproject/DeviceLocation#113 ;)

@maxl2287
Copy link
Contributor Author

@bigludo7 what about:

    SubscriptionDetail:
      description: The detail of the requested event subscription
      type: object
      required:
        - type
      properties:
        device:
          $ref: "#/components/schemas/Device"
        type:
          $ref: "#/components/schemas/SubscriptionCreationEventType"

To reduce redundancy?

@bigludo7
Copy link
Collaborator

SubscriptionDetail

Hello @maxl2287
Yes. Sorry for the proposal - we can simplify.

My only request is to have a distinct array for the type attribute in SubscriptionDetail from the array in CloudEvent.

@maxl2287
Copy link
Contributor Author

@bigludo7 sorry I have inserted the wrong code as a proposal 😄 .

I mean:
Is this here a good way to reduce redundancy:

    SubscriptionEventType:
      allOf:
        - $ref: "#/components/schemas/SubscriptionCreationEventType"
        - type: string
          description: |
            subscription-ends: Event triggered when the subscription ends.
          enum:
            - org.camaraproject.device-status.v0.subscription-ends

@maxl2287 maxl2287 requested a review from bigludo7 November 14, 2023 08:42
@bigludo7
Copy link
Collaborator

@bigludo7 sorry I have inserted the wrong code as a proposal 😄 .

I mean: Is this here a good way to reduce redundancy:

    SubscriptionEventType:
      allOf:
        - $ref: "#/components/schemas/SubscriptionCreationEventType"
        - type: string
          description: |
            subscription-ends: Event triggered when the subscription ends.
          enum:
            - org.camaraproject.device-status.v0.subscription-ends

Yes this is excellent. I should confess that I did not know that it was possible to extend an enum this way. Thanks !!

bigludo7
bigludo7 previously approved these changes Nov 14, 2023
Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks @maxl2287

sachinvodafone
sachinvodafone previously approved these changes Nov 14, 2023
@maxl2287
Copy link
Contributor Author

maxl2287 commented Nov 14, 2023

@bigludo7 sorry I have inserted the wrong code as a proposal 😄 .

I mean: Is this here a good way to reduce redundancy:

SubscriptionEventType:
  allOf:
    - $ref: "#/components/schemas/SubscriptionCreationEventType"
    - type: string
      description: |
        subscription-ends: Event triggered when the subscription ends.
      enum:
        - org.camaraproject.device-status.v0.subscription-ends

Yes this is excellent. I should confess that I did not know that it was possible to extend an enum this way. Thanks !!

I don't know. I thought so, but in DeviceLocation @jlurien denied that possibility.

So I guess I need to revert it.

@maxl2287 maxl2287 changed the title add /connectivity - endpoint to check the network status of a device Draft: add /connectivity - endpoint to check the network status of a device Nov 14, 2023
@maxl2287 maxl2287 dismissed stale reviews from sachinvodafone and bigludo7 via 7fb6513 November 15, 2023 08:33
@maxl2287 maxl2287 force-pushed the draft/check-status-for-device branch from 3b6cffa to 7fb6513 Compare November 15, 2023 08:33
@maxl2287
Copy link
Contributor Author

@bigludo7 I have reverted my commit and deleted an unused component.
Now I think its good to be reviewed.

I was not aware that an "extend" is not possible for enums yet and it's only possible by a bit of redundancy.

@maxl2287 maxl2287 changed the title Draft: add /connectivity - endpoint to check the network status of a device add /connectivity - endpoint to check the network status of a device Nov 15, 2023
Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @maxl2287

@bigludo7
Copy link
Collaborator

Hello @sachinvodafone @eric-murray
Kind reminder to get your review of the PR. Thanks.
cc: @akoshunyadi

@bigludo7 bigludo7 merged commit f126f8c into camaraproject:main Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Endtype Connectivity
5 participants