search

ivoa-std / SSO

Single Sign On
Creative Commons Attribution Share Alike 4.0 International
1 stars 6 forks source link

SSO Next (2.1 or 3.0) #10

Closed brianmajor closed 2 years ago

brianmajor commented 2 years ago

Please refer to https://wiki.ivoa.net/twiki/bin/view/IVOA/SSO_next for the source of changes.

brianmajor commented 2 years ago

Hi @bertocco - In the future I suggest we use the comment and review process in github PRs to discuss the changes in the PR, and not merge until all (or most) authors have taken a look and approved. That way you can put the review comments and suggestions right by the new or altered text.

The changes in this particular PR still need some work. As we discussed at the GWS session, I think someone could try to rearrange the document so it better reflects the new information we've put in.

Cheers, Brian

bertocco commented 2 years ago

Sorry, but in my habit the discussion takes place in the issues and when the pull request is done the work is done. In any case, for the next time, what triggers the merge? Cheers, Sara

On Wed, May 4, 2022 at 8:41 PM Brian Major @.***> wrote:

Hi @bertocco https://github.com/bertocco - In the future I suggest we use the comment and review process in github PRs to discuss the changes in the PR, and not merge until all (or most) authors have taken a look and approved. That way you can put the review comments and suggestions right by the new or altered text.

The changes in this particular PR still need some work. As we discussed at the GWS session, I think someone could try to rearrange the document so it better reflects the new information we've put in.

Cheers, Brian

— Reply to this email directly, view it on GitHub https://github.com/ivoa-std/SSO/pull/10#issuecomment-1117679554, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKEFHCPAA2WIUQ24WHFCD3VILAHJANCNFSM5UKABEZA . You are receiving this because you were mentioned.Message ID: @.***>

brianmajor commented 2 years ago

No worries Sara. I think you must have pressed the green 'Merge' button after approving the pull request?

brianmajor commented 2 years ago

I'm just realizing now that you are asking 'when should we decide to merge'? Good question. Each reviewer can create a number of issues. These can be marked as 'resolved' by either the reviewer or PR author. Once a satisfying number of reviews with approvals (with no unresolved issues) have been made the author could post something like "This PR has been reviewed and is ready for merging". Thoughts?

Zarquan commented 2 years ago

I think we will have 2 different kinds of PRs. Simple changes to fix specific issues, like #15, in which case the author of the PR should invite the document editor to review and the editor can go ahead and merge it when they are happy with it.

Then there will be more complex PRs which may need to be reviewed by a larger group. In which case, the author of the PR should add a comment to say "this PR needs review by several people" and add everyone concerned to the reviewers list. The document editor can also request additional reviews from other people if they think it is appropriate. The document editor merges the PR when they are happy that the discussion has reached a consensus and all the issues have been resolved.

In both cases it should be up to the document editor to decide when to merge a PR.