-
Notifications
You must be signed in to change notification settings - Fork 48
fix: update-signals-sdk #466
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
Conversation
|
View your CI Pipeline Execution ↗ for commit ec30a93
☁️ Nx Cloud last updated this comment at |
5f80a0e to
f2c14d9
Compare
🦋 Changeset detectedLatest commit: ec30a93 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #466 +/- ##
===========================================
- Coverage 58.68% 56.93% -1.76%
===========================================
Files 105 105
Lines 25680 31600 +5920
Branches 1690 1785 +95
===========================================
+ Hits 15070 17991 +2921
- Misses 10610 13609 +2999 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
packages/ping-protect/package.json
Outdated
| "types": "./dist/index.ts.d.ts", | ||
| "dependencies": { | ||
| "@forgerock/javascript-sdk": "workspace:^" | ||
| "@forgerock/javascript-sdk": "^4.8.2" |
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.
why this change?
packages/ping-protect/package.json
Outdated
| { | ||
| "name": "@forgerock/ping-protect", | ||
| "version": "4.6.0", | ||
| "version": "4.6.0-beta.1", |
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.
?
168267e to
6dc6b7a
Compare
6dc6b7a to
9ab3822
Compare
commit: |
|
|
||
| // Optional parameters | ||
| agentIdentification: this.getOutputByName<boolean>('agentIdentification', false), | ||
| agentTimeout: this.getOutputByName<number>('agentTimeout', 0), |
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.
When testing these changes the fact that these values are being defaulted to 0 is causing issues as this is setting the port and timeout to 0 (Meaning the agent does not get connected to).
When the values are not set in the node the expectation is that we should be using the default values, which is in line with the signals SDK init call expecting these values to be present ONLY if the default values are not desired - see documentation here https://apidocs.pingidentity.com/pingone/native-sdks/v1/api/#pingone-protect-sdk-for-web:~:text=//%20If%20you%20have,9400)%0A%20%20%20%20%20%20%20%20//%20agentPort%3A%208800
If default values have to be provided in this class, perhaps we should use the Signals SDK default values here instead?
Default values for these configuration options are:
agentPort= 9400agentTimeout= 1000
(Default values inferred from documentation here: https://docs.pingidentity.com/pingone/strong_authentication_mfa/p1_using_the_workforce_trust_agent.html#:~:text=the%20agent%20timeout%20(-,1000%20ms,-)%2C%20use%20the%20PingID )
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.
Okay, I can make the changes. Rather than dealing with default values, I'll just remove the config property if it's not on the callback. Would that work better?
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.
That would be great - keeps things in line with the Signals expected SDK behaviour and if they update their defaults we don't need any additional update.
Thanks
9ab3822 to
6dc6b7a
Compare
- revert small change to ping-protect package.json - fix e2e tests - conditionally add "agent" properties on callback only when present
6dc6b7a to
ec30a93
Compare
|
Tested these changes with the new beta and things are looking good - thanks for the swift turnaround on this. ✅ |
JIRA Ticket
Please link jira ticket here
Description
Type of Change
Please Delete options that are not relevant
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Definition of Done
Check all that apply
Documentation