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

Handle CORS for GetValidated3pidServlet #342

Merged

Conversation

PiotrKozimor
Copy link
Contributor

@PiotrKozimor PiotrKozimor commented May 24, 2021

Pull Request Checklist

  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fix a bug that prevented receiving messages from other servers." instead of "Move X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
  • Pull request includes a sign off

Signed-off-by: Piotr Kozimor p1996k@gmail.com

@PiotrKozimor
Copy link
Contributor Author

It looks like buildkite/sydent/rolled-up-newspaper-newsfile still takes master branch instead of main.

@PiotrKozimor PiotrKozimor marked this pull request as ready for review May 24, 2021 15:17
@PiotrKozimor
Copy link
Contributor Author

Kindly asking for review :)@anoadragon453

@callahad
Copy link
Contributor

@anoadragon453 As you just did this for Sygnal... would kindly repeat your CI pipeline + towncrier magic (master -> main) for this?

@richvdh
Copy link
Member

richvdh commented May 25, 2021

we did both together. Have restarted the builds here.

@callahad
Copy link
Contributor

We just ran black on the codebase, which unfortunately produced a merge conflict here.

Could you please rebase or merge main here?

@PiotrKozimor PiotrKozimor force-pushed the piotr-kozimor/getvalidated3pid-cors branch from 34edcd2 to 2ecb4a0 Compare May 27, 2021 06:57
@PiotrKozimor
Copy link
Contributor Author

Rebase done, we have no conflicts.

@PiotrKozimor
Copy link
Contributor Author

Is anything blocking review @anoadragon453 ? Was there any reason not to put CORS in this endpoint? Or was is simply forgotten?

@callahad
Copy link
Contributor

callahad commented Jun 1, 2021

We should be able to get back to this within the next 2 days at worst -- we're just coming off of a bank holiday weekend. I'll need to do some digging to make sure I'm not missing a reason that CORS headers were not present on this endpoint.

Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

Hey @PiotrKozimor, apologies for the delay on getting to this. The send_cors addition to the codebase dates back to the very first commit, whereas the addition of the servlet was later (without much explanation).

I don't see any problem with adding CORS headers to this endpoint, so I'm going to go ahead and chalk it up to "forgetfulness".

Just a couple things with the changelog, otherwise the rest of the code looks good to me! (and note that there's now another merge conflict).

changelog.d/PR342.bugfix Outdated Show resolved Hide resolved
changelog.d/PR342.bugfix Outdated Show resolved Hide resolved
PiotrKozimor and others added 3 commits June 7, 2021 09:45
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
@PiotrKozimor
Copy link
Contributor Author

Now it was me having long holidays 🙃 I have pushed requested changes.

Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

LGTM. Test failures are unrelated to this change. Thanks again!

@anoadragon453 anoadragon453 merged commit af32e62 into matrix-org:main Jun 7, 2021
@PiotrKozimor PiotrKozimor deleted the piotr-kozimor/getvalidated3pid-cors branch June 7, 2021 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants