camaraproject / QualityOnDemand

Repository to describe, develop, document and test the QualityOnDemand API family
https://wiki.camaraproject.org/x/zwOeAQ
Apache License 2.0
41 stars 59 forks source link

issue_194_PR #217

Closed SyeddR closed 2 months ago

SyeddR commented 1 year ago

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #194

Special notes for reviewers:

For lack of the better term i am proposing "applicationConsumerId" as string type field. Open for suggestions

Changelog input

 release-note

Additional documentation

This section can be blank.

docs
eric-murray commented 1 year ago

Hi @SyeddR

I had understood that the applicationConsumerId must be associated with one, and only one application. Is that the case? If so, my preference would be for this to be a sub-property of the applicationServer property, the reason being that I'm not a fan of "flat" input parameter structures, and much prefer that related parameters be grouped, particularly when some (such as this one) are optional.

Of course, you could argue that this should also apply to applicationServerPorts, and I'd agree with you :-)

I also agree that adding this property to the ApplicationServer schema requires some thought, as we need to ensure that the API consumer provides at least one IP address, but there are ways in OAS that that can be mandated whilst still allowing applicationConsumerId to be optionally specified.

SyeddR commented 1 year ago

@eric-murray yes applicationConsumerId must be associated with one and only one application. I dont have a strong opinion on keeping it under applicationServer property or outside of it. Either way works for me.

jlurien commented 1 year ago

I suggested to open an issue on Identity and Consent WG about this topic, as identification of Application is being discussed there in the context of authentication, and access token should contain all required information.

SyeddR commented 11 months ago

As per the action items from the discussion in the last meeting.

  1. Changed the naming from applicationConsumerId to applicationId to avoid confusion with the consumer of the application.
  2. Added more description for the purpose of applicationId and how it is different than a clientId.
  3. Moved the parameter under application server object.
eric-murray commented 11 months ago

@SyeddR I'm fine with the proposal, but updated the ApplicationServer schema description to clarify that applicationId itself is not sufficient to identify the application server, and that an IP address is still required.

I'm a bit concerned that the schema definition will not catch API consumers who only provide the applicatonId (because that will satisfy the minProperties: 1 directive), so am wondering if we need to re-organise the schema so that a single ipAddress property is now required. One for discussion.

There appear to be some conflicts now with the main branch (which maybe I introduced - apologies if that is the case), Can you synchronise your fork with main to see if that fixes things?

SyeddR commented 11 months ago

There appear to be some conflicts now with the main branch (which maybe I introduced - apologies if that is the case), Can you synchronise your fork with main to see if that fixes things?

@eric-murray My fork shows all synced with latest updates. Can you try to resolve the conflict at your end.

eric-murray commented 11 months ago

@SyeddR OK, fixed and now approved from my side. It seems my original edits were not properly merged into your own fork, but subsequent edits were, so all good now.

hdamker commented 11 months ago

Converted to draft status as agreed within our call on Nov 3rd.

hdamker commented 2 months ago

This PR is stale and can't be merged into the current (splitted) API structure anymore.