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

Fetch device from IS for Unclaim and GetClaimStatus RPCs #6325

Closed
wants to merge 8 commits into from

Conversation

KrishnaIyer
Copy link
Member

@KrishnaIyer KrishnaIyer commented Jun 14, 2023

Summary

References; https://github.com/TheThingsIndustries/product-management/issues/6

Changes

  • Remove the request body from Unclaim. We are doing this since the support for a request body for HTTP Delete requests is unclear.
    • Also, we have to get the device from the IS to check that the EUIs are correct anyway.
    • This is a break in API but only the console/CLI currently use it and @adriansmares and I feel that this is justified.
  • Get the EUIs for devices from the IS instead of using the request values for Unclaim and GetClaimStatus.
  • Update tests
  • Update console.

Testing

DCS locally tested. For the console part, I think if the Cypress tests pass, I think this is good.

Regressions

Will be tested on staging1 since there's currently no support for local TTJS testing.

Notes for Reviewers

⚠️ We are (mildly) breaking API here. The request URI for unclaim always needed to include the App and Device ID. It's just that we no longer accept a body.

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.

@KrishnaIyer KrishnaIyer added this to the v3.26.2 milestone Jun 14, 2023
@KrishnaIyer KrishnaIyer self-assigned this Jun 14, 2023
@github-actions github-actions bot added compat/api This could affect API compatibility ui/web This is related to a web interface labels Jun 14, 2023
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.

We should remove that allow_delete_body from the proto generator as well.

sdk/js/src/service/claim.js Show resolved Hide resolved
@KrishnaIyer KrishnaIyer force-pushed the feature/3632-break-unclaim-api branch from 5154d5c to b436979 Compare June 14, 2023 12:34
@github-actions github-actions bot added the tooling Development tooling label Jun 14, 2023
@KrishnaIyer KrishnaIyer force-pushed the feature/3632-break-unclaim-api branch from 7106551 to de8fcb2 Compare June 16, 2023 12:12
@KrishnaIyer KrishnaIyer marked this pull request as ready for review June 16, 2023 12:38
@KrishnaIyer
Copy link
Member Author

The console part is a "best effort". Let me know what else needs to be changed.

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.

API and DCS LGTM

@johanstokking johanstokking added the security This is important for security label Jun 17, 2023
Copy link
Member

@kschiffer kschiffer left a comment

Choose a reason for hiding this comment

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

Codeowned LGTM.
Mildly related: #6333, but now we don't use any parameters here (other than route parameters) so it should be fine either way.

Copy link
Contributor

@nicholaspcr nicholaspcr left a comment

Choose a reason for hiding this comment

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

LGTM

@johanstokking
Copy link
Member

  • Also, we have to get the device from the IS to check that the EUIs are correct anyway.

As discussed offline; if the device does not exist, DCS should just proceed attempting to unclaim.

@KrishnaIyer KrishnaIyer force-pushed the feature/3632-break-unclaim-api branch from de8fcb2 to 8ceab0d Compare June 26, 2023 11:41
@github-actions github-actions bot removed the security This is important for security label Jun 26, 2023
@KrishnaIyer KrishnaIyer modified the milestones: v3.26.2, v3.27.0 Jun 30, 2023
@adriansmares adriansmares changed the base branch from v3.26 to v3.27 July 3, 2023 10:56
@KrishnaIyer KrishnaIyer mentioned this pull request Jul 5, 2023
5 tasks
@KrishnaIyer
Copy link
Member Author

I'll close this PR but keep the branch. I'll adapt some of these changes after #6381 is merged.

@KrishnaIyer KrishnaIyer deleted the feature/3632-break-unclaim-api branch August 4, 2023 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compat/api This could affect API compatibility tooling Development tooling ui/web This is related to a web interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants