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

Update from notificationsUri to notificationsUrl #89

Merged
merged 1 commit into from
Jan 18, 2023
Merged

Update from notificationsUri to notificationsUrl #89

merged 1 commit into from
Jan 18, 2023

Conversation

maxl2287
Copy link
Collaborator

fixes #88

@maxl2287 maxl2287 changed the title update to from notificationsUri to notificationsUrl Update to from notificationsUri to notificationsUrl Jan 16, 2023
@hdamker
Copy link
Collaborator

hdamker commented Jan 16, 2023

LGTM, at least one further review from @eric-murray @jlurien or @patrice-conil would be good.

@hdamker hdamker added bug Something isn't working QoD labels Jan 16, 2023
@hdamker hdamker changed the title Update to from notificationsUri to notificationsUrl Update from notificationsUri to notificationsUrl Jan 16, 2023
patrice-conil
patrice-conil previously approved these changes Jan 17, 2023
@eric-murray
Copy link
Collaborator

I had a couple of questions / comments on this PR:

  • Why are modifications being proposed for the "legacy" implementation within the main QualityOnDemand repository rather than moving the implementation to the new QualityOnDemand_PI1 repository? I had understood that implementations would no longer be stored within the main API repository.
  • A change is being proposed to a copy of v0.1.0 stored within the implementation, but no change is being proposed to the version number of that copy. This is potentially confusing if such files are viewed out of context. I'd prefer if modifications made for the purposes of an implementation have a suitably extended version number. I'd propose a company specific extension to the base version number (e.g. 0.1.0-dtag.1) when such changes are made. This is the approach that we took within Vodafone when modifying the API specification for our specific implementation (e.g. update of apiRoot and addition of OAuth scope).

@jlurien
Copy link
Collaborator

jlurien commented Jan 17, 2023

I'm OK with renaming parameter to notificationUrl, but I agree with @eric-murray. I think we should fix code/API_definitions/qod-api.yaml and doc here, and move implementations to the new repos.

We could upgrade yaml version to 0.8,1 and take advantage of the change to incorporate the other typos identified.

@maxl2287
Copy link
Collaborator Author

@jlurien and @eric-murray thanks for your feedback.
I will then revert the implementation changes and just change the qod-api.yaml.

@hdamker
Copy link
Collaborator

hdamker commented Jan 17, 2023

@maxl2287 please keep also the change in documentation. And I recommend not to touch the version number in the spec file until we decide to make a new release (like v0.8.1).

@jlurien
Copy link
Collaborator

jlurien commented Jan 17, 2023

@maxl2287 please keep also the change in documentation. And I recommend not to touch the version number in the spec file until we decide to make a new release (like v0.8.1).

@hdamker Should we resolve the release first and then apply this change towards 0.8.1?

@maxl2287
Copy link
Collaborator Author

Branch is now updated without implementation changes

@eric-murray
Copy link
Collaborator

@maxl2287 please keep also the change in documentation. And I recommend not to touch the version number in the spec file until we decide to make a new release (like v0.8.1).

@hdamker Will you create the release v0.8.0 first before merging this PR? Although the change is small, our current implementation uses the current v0.8.0 and not this update, and anyone looking for v0.8.0 in the github should find the current version and not subsequent updates that have not incremented the version number.

@hdamker
Copy link
Collaborator

hdamker commented Jan 17, 2023

@eric-murray

Will you create the release v0.8.0 first before merging this PR?

Yes, will do today. I have exactly the same intention to get the v0.8.0 release before any further fix merged.

And, to answer the other comments: the v0.1.0 implementation code will not be part of the v0.8.0 release (deleted it from the repo already).

@hdamker
Copy link
Collaborator

hdamker commented Jan 18, 2023

@jlurien @patrice-conil can you approve (again)? Then we should be ready here.

Copy link
Collaborator

@jlurien jlurien left a comment

Choose a reason for hiding this comment

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

LGTM

@hdamker hdamker merged commit 5ce0f8a into camaraproject:main Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QoD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consistent usage of notificationUri vs notificationUrl
6 participants