-
Notifications
You must be signed in to change notification settings - Fork 4
v2.0.0-rc.1 #123
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
v2.0.0-rc.1 #123
Conversation
|
@FabrizioMoggio Great to see the release PR, I will create the release review issue. It would be good if you:
That will make the release review easier. |
|
LGTM |
JoseMConde
left a comment
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
'Changed' issues where originally under 'Added'
|
@hdamker I guess you mean me 😁 Great to see the release PR, I will create the release review issue. It would be good if you:
Thanks! Changed API Readiness Checklist API version, added version to Feature name
I've added them as links after the relevant issues (those which affect API consumers) but let me know if the formatting should be changed. |
@Kevsy yep, yes I meant you ... too many comments written already. Regarding the format: the important point is here that readers of the CHANGELOG have links to the PRs which they can easily open to see what actually was changed in the files to address the point (that also the reasons why one PR should address normally one issue/topic). There is no defined format, but most teams follow the format which is created by GitHub if you use "Generate release notes" e.g.
Normally you have to rewrite by something from the "release note" part of the PR (if provided at all). Could be also that you would list the PR multiple times if it covered several issues. Important point here is to have the full link to the PR, not only a relative one, so that the links work as well if someone downloaded the zip file or has cloned the repository locally. |
|
Thanks @hdamker , PR titles and absolute links now updated per format:
...and with absolute URIs. |
| security: | ||
| - openId: | ||
| - simple-edge-discovery:edge-cloud-zones:read | ||
| - simple-edge-discovery:read |
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.
Just for curiosity: why this (breaking) change? (I guess both options are correct)
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.
Just for curiosity: why this (breaking) change? (I guess both options are correct)
@hdamker changed because the Design Guide says:
"
-
api-name is the API name specified as the base path, prior to the API version, in the servers[*].url property. For example, from /location-verification/v1, it would be location-verification.
-
resource is optional. For APIs with several paths, it may include the resource in the path. For example, from /qod/v1/sessions/{sessionId}, it would be sessions."
"
Since SED has only one path, the optional resource does not apply according to the wording above. As this release is breaking anyway (because of the move from GET to POST) it seems a good time to make the scope name compliant at the same time :)
|
Typo detected in
|
|
@rartych I updated the README to include more Release Information, at least until the template is agreed. |
|
@Kevsy Please commit the accidental link removal, then we can go with the release. |
rartych
left a comment
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
|
Thanks @rartych - |
Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>
rartych
left a comment
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.
Re-approved
JoseMConde
left a comment
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. Re-approved.
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
This is the first release candidate PR for SED v2.0.0 . It only includes the updates necessary for a release PR:
Which issue(s) this PR fixes:
Fixes #
Special notes for reviewers:
Changelog input
Additional documentation
This section can be blank.