-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
✨ [RTC-Config]: add source_url and title to real-time-config global macros allowlist #38723
Conversation
Hi @calebcordry , could I have some assistance with getting this PR over the line? :) |
@zshnr apologies for the delay. could you rebase this issue. we'll try and get this over the line |
e3933df
to
54ee481
Compare
@erwinmombay Thank you so much! I have rebased off of the main branch, fixed the one conflict, and pushed the changes :) |
@erwinmombay I might need help debugging the unit tests. Not sure how I can log the output of the function called in the failing test to see in either the browser console or command line. Is there anyone that can assist with this? Thanks! |
I would start with generalizing your regex and see at which point would the unit test passes. |
@powerivq I have updated the test's regex and it passes. However, I had to generalize the part of the regex that checks that SOURCE_URL is a valid url with either http or https. old regex had
generalised this with:
Is this testing for the correct behavior of SOURCE_URL substitution or does this point to SOURCE_URL not substituting the correctly formatted url string? |
@zshnr I think escaping might have occur and failed your regex. |
…acros allowlist (ampproject#38723) * add source_url and title to real-time-config global macro allowlist * add back accidently removed regex token * fix unit test
Implements the changes proposed in #38627 by adding the TITLE and SOURCE_URL global macros to the allowlist for real-time-config