-
Notifications
You must be signed in to change notification settings - Fork 60
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
Conversation
LGTM, at least one further review from @eric-murray @jlurien or @patrice-conil would be good. |
I had a couple of questions / comments on this PR:
|
I'm OK with renaming parameter to We could upgrade yaml version to 0.8,1 and take advantage of the change to incorporate the other typos identified. |
@jlurien and @eric-murray thanks for your feedback. |
@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). |
Branch is now updated without implementation changes |
@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. |
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). |
@jlurien @patrice-conil can you approve (again)? Then we should be ready here. |
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
fixes #88