-
Notifications
You must be signed in to change notification settings - Fork 54
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
Elisa/7492 public device endpoint #8341
base: main
Are you sure you want to change the base?
Conversation
870dfba
to
6fd0a9b
Compare
6fd0a9b
to
42aaf8b
Compare
Quality Gate passedIssues Measures |
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.
Tested and the end point works as expected with and without authorization + paired with Elisa and talked through all of my questions -- LGTM! Thanks for all of you work on this Elisa!!
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.
Tested the endpoint and everything looks good!
With the current configuration it looks like the lower environments will also end up having this endpoint active right? Just want to check whether that is desired or whether we should have a configuration setting so that by default the endpoint is disabled in the lowers but could be turned on for testing if needed similar to the sendgrid config setting?
private int testLength; | ||
|
||
@JsonIgnore |
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.
@emyl3 Have you looked into our concerns that removing these ignore tags will cause an issue?
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.
And I apologize because I just now realized I never added that detail to the ticket:
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 had previously deployed this branch both with and without the @JsonIgnore
-- is this what you were thinking in terms of adding a timer and logging to see if there's a difference with and without the @JsonIgnore
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.
Let me know if this is not what you had in mind and I can adjust the testing plan and retest 😅
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.
Part of me thinks that the issue is no longer relevant because the audit log was moved out of our db table. The other part of me thinks if it is somehow still an issue, we aren't going to see it until we send this to prod.
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 talked to @mehansen about this a bit and I loved her idea of just trying to replicate the original issue as a means of confirming that its no longer a concern. But again, the lack of an audit log in the db might make that a futile effort.
BACKEND PULL REQUEST
Related Issue
Changes Proposed
/devices
that returns allDeviceType
s (without any internal ids)@JsonView
annotation to return certain fieldsSr-Prod-Devices-Token
with the valid token (See Add prod device sync related secrets to tf configs #8323 for how to get the token value)Additional Information
DeviceTypeProdSyncService
for logic related to the device sync from prodTesting
dev4
dev4
e.g.
Sr-Prod-Devices-Token
headere.g.
Unauthorized