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

XEP-0371: migrate from RFC 5245 to 8445 #905

Merged
merged 1 commit into from Oct 20, 2020
Merged

Conversation

Ri0n
Copy link
Contributor

@Ri0n Ri0n commented Mar 11, 2020

Changes:

  • RFC 5245 is replaced with RFC 8445
  • Introduced ice2 transport attribute for backward compatibility, same as in RFC 8445
  • Clarified ICE restart procedure
  • Make remote-candidate MUST when urn:ietf:rfc:3264 is advertised by the responder
  • Send remote-candidate for all components at once to be better compatible with SIP gateways.
  • Wrong reference to RFC 6455 was replaced with correct one: RFC 6544
  • Allow gathering-complete element to be combined with remaining candidates

@Ppjet6 Ppjet6 added the Needs Author The XEP is experimental and the PR was not made by the author. The author needs to acknowledge it. label Mar 11, 2020
@Ri0n Ri0n force-pushed the patch-8 branch 2 times, most recently from 4d516ca to 06bf128 Compare March 21, 2020 15:51
@dwd
Copy link
Contributor

dwd commented May 14, 2020

Paging @stpeter

@Ri0n
Copy link
Contributor Author

Ri0n commented Aug 19, 2020

ping

@stpeter
Copy link
Member

stpeter commented Aug 19, 2020

These changes seem fine (I might send the author an email or provide a PR with a few editorial suggestions). Because we're adding the 'ice2' attribute, I wonder if we need a namespace bump - especially because aggressive nomination is no longer supported (however, 'ice2' defaults to false, so this might not be necessary).

@Ri0n
Copy link
Contributor Author

Ri0n commented Aug 26, 2020

ping @horazont

@horazont horazont added Ready To Merge No acknowledgements of other parties are needed anymore. There may be changes to do at merge time. and removed Needs Author The XEP is experimental and the PR was not made by the author. The author needs to acknowledge it. labels Aug 26, 2020
@horazont
Copy link
Contributor

horazont commented Oct 6, 2020

@Ri0n Eh, sorry, this slipped through. Fun times.

So, problem here: you added another commit after the approval. In addition, the commit edits the already-existing version block, which is not allowed.

I would suggest that you remove that commit and create a separate PR for the gathering changes there.

@Ri0n
Copy link
Contributor Author

Ri0n commented Oct 6, 2020

I was asked in @xsf (IIRC by you @horazont) to add another commit to change the date for changelog. I did.

Do you mean I have to split this one into two? First one - all the changes but without "changes" part and another other just "changes"?

@Ri0n
Copy link
Contributor Author

Ri0n commented Oct 6, 2020

Looks like force-push showed wrong history here on github

@horazont
Copy link
Contributor

horazont commented Oct 6, 2020

@Ri0n The one problem is that the newest commit must be the one adding the revision block, because that is the one which will be tagged to constitute the release of the new version. So you could squash them. The other problem is that the commit where you changed the date does more than just changing the date: c589749

So this is what confuses me; and this seems to be another non-editorial change we should ask the original authors about. Then again, I’m still pretty tired so I wonder if I’m missing something here.

@Ri0n
Copy link
Contributor Author

Ri0n commented Oct 6, 2020

Probably I did git --amend instead of a new commit. To be honest I don't remember why,
But those extra changes in the latest commit were definitely there before approval.

@horazont
Copy link
Contributor

horazont commented Oct 6, 2020

@Ri0n Thanks, that makes sense. I’ll clean that up during the merge next week.

@horazont horazont self-assigned this Oct 6, 2020
* Replaced RFC 5245 with RFC 8445
* Introduced ice2 transport attribute for backward compatibility
* Clarified ICE restart procedure
* Clarified remote-candidate usafe with "urn:ietf:rfc:3264"
* Changed remote-candidate notification procedure (sent all at once now)
* Replaced wrong reference to RFC 6455 with correct one: RFC 6544
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready To Merge No acknowledgements of other parties are needed anymore. There may be changes to do at merge time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants