-
Notifications
You must be signed in to change notification settings - Fork 137
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 event listener support to the Desktop Agent API #1207
Conversation
✅ Deploy Preview for fdc3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
@kemerava Many thanks for the contribution here! I've made a few suggestions to tighten it up.
Please note the comment from EasyCLA, we'll need you to click on the authorization link and to make sure your firm has a CCLA in place (contact fdc3-maintainers@finos.org and help@finos.org if you have any issues there). I also note one of the commits has an issue with a missing github id that will also need to be fixed or the PR recreated without it.
a1f7cb5
to
4669b0e
Compare
Co-authored-by: Kris West <kris.west@interop.io>
c4cbb48
to
0b2620f
Compare
Thank you for the review, @kriswest! |
If the CCLA is in place, you still need to click on the Please click here to be authorized link - that will confirm your status on the CCLA and future updates won't require it. In case you've already done that, I'll see if I can get easyCLA to recheck via a comment - failing that we may have to ask their support for help (or further assistance with EasyCLA, please submit a support request ticket.) Glad you got the other error resolved! |
/easycla |
Thank you, @kriswest! Thanks for the info on the CCLA part, I will bring it back to my team to let them know |
I opened one as well @kemerava: https://jira.linuxfoundation.org/plugins/servlet/desk/portal/4/SUPPORT-26910 They may know what's up... but also made a comment that suggests you haven't yet hit the 'click here to be authorized' link in the comment the EasyCLA bot made above and selected proceed as a corporate contributor and been approved - if you have, add a comment here so they see it. |
Also if you visit: https://patch-diff.githubusercontent.com/raw/finos/FDC3/pull/1207.patch you'll see the emial it is picking up for you (which may not be the right one to get you approved on a corporate CLA) |
/easycla |
@kemerava they rolled back easyCLA and it is running checks again properly - it cleared one of the errors so I know its updated. Now it just needs to know which CCLA you are on - so go hit that Please click here to be authorized link and complete the steps. If you still get nowhere then maybe add your work email to your github account if you haven't done so already - the email domain is often used as the approval criteria by CLA managers. Alternatively, your firm's CLA manager can manually add you to the CCLA. |
Hi @kriswest, Just finished figuring out the CLA from our side. Looks like there are no more errors from the easyCLA side. |
@kemerava looks good to me - we got consent at today's meeting to go ahead with this once it passes a couple of reviews, which will hopefully happen over the next couple of weeks. Again, many thanks for the contribution! |
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.
All comments are nitpicks about backticks. Content looks good, matches the proposed API in the issue.
Co-authored-by: Hugh Troeger <troeger.hugh@gmail.com>
LGTM and matches the proposal. Applied your suggestions @hughtroeger after figuring out that you need to use quadruple backticks in github suggestions involving triple backticks! I was involved in the proposal, so adding other maintainers to review and will leave merging a week or two to facilitate. |
Hi @kriswest, any updates on this? Anything needed from my side? Thank you! |
Hi @kemerava, nothing more is needed apart from a review from another maintainer (which it looks like I forgot to request - done now)! |
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.
Hi @kriswest, |
Looks like this is all green. What is holding this PR up from merge? |
@michael-bowen-sc now that Brian and Vinay have had a look it's all good to go. Apologies for the delay, OSFF prep was.distractong me! |
…n-tests Corrections to #1207 to fix broken tests
This PR when in without a changelog entry, which we will need to add before the next version of FDC3 is adopted |
Closes #1136
Adding support for event listener for non-context and non-intent events
@michael-bowen-sc