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

amp-o2-player added consent data handling #30858

Closed
wants to merge 4 commits into from

Conversation

lironka
Copy link
Contributor

@lironka lironka commented Oct 26, 2020

✨ Added consent data handling to amp-o2-player

@CLAassistant
Copy link

CLAassistant commented Oct 26, 2020

CLA assistant check
All committers have signed the CLA.

@amp-owners-bot amp-owners-bot bot requested a review from nainar October 26, 2020 13:31
@amp-owners-bot
Copy link

amp-owners-bot bot commented Oct 26, 2020

Hey @alanorozco! These files were changed:

extensions/amp-o2-player/0.1/amp-o2-player.js
extensions/amp-o2-player/0.1/test/test-amp-o2-player.js
extensions/amp-o2-player/amp-o2-player.md

@lironka
Copy link
Contributor Author

lironka commented Oct 28, 2020

@nainar I apologize for the urgency, but could you please merge this pr at your earliest possible convenience? It is impact on business a lot.

@micajuine-ho micajuine-ho requested review from micajuine-ho and removed request for nainar October 28, 2020 17:17
Copy link
Contributor

@micajuine-ho micajuine-ho left a comment

Choose a reason for hiding this comment

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

Hi @lironka thank you for your PR. It looks good to me, however, I want to confirm that you did some manual testing before we merge.

@lironka
Copy link
Contributor Author

lironka commented Oct 31, 2020

@micajuine-ho hi! I did manual testing and it worked well.

Copy link
Contributor

@micajuine-ho micajuine-ho left a comment

Choose a reason for hiding this comment

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

Awesome! Sounds great :D.

@micajuine-ho micajuine-ho reopened this Nov 1, 2020
@lironka
Copy link
Contributor Author

lironka commented Nov 1, 2020

@micajuine-ho Hi! Could you, please, merge the pr? I haven't permission to do it.

@micajuine-ho
Copy link
Contributor

Hi ! @lironka, can you force merge a blank/trivial commit. It seems like the Travis job is stuck/not loading, so we need to trigger it again.

Additionally, the owners check should not be an issue here, since wg-components is also listed as an owner, and I have given approval.

@lironka
Copy link
Contributor Author

lironka commented Nov 3, 2020

@micajuine-ho HI! I updated but travis still looks as waiting. Do you know what could we do?

@micajuine-ho
Copy link
Contributor

I'm not sure why this is happening. Maybe best shot is just to close this PR and open a new one...

@estherkim
Copy link
Collaborator

Hi @lironka, my guess is that Travis is stalling because we migrated from Travis.org to Travis.com on Oct 23, which isn't included in this branch.

Please merge this branch with HEAD for the latest Travis config and let's see how that goes.

@lironka
Copy link
Contributor Author

lironka commented Nov 4, 2020

@estherkim Hi! I have latest master and don't see any brach with name HEAD. What I need to do for "merge this branch with HEAD" ?

@estherkim
Copy link
Collaborator

Hi, what I meant was to get latest (the HEAD commit) from master and merge it to this branch. It looks like this branch was created on Oct 22 and hasn't been merged with master since. Here are the steps to merge with latest -

https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#pull-the-latest-changes-from-the-amphtml-repository

@lironka
Copy link
Contributor Author

lironka commented Nov 4, 2020

@estherkim Hi! thank you I updated. But still my upstream/master has travis.org not travis.com as you mentioned. Are you sure travis.com was merged?

@estherkim
Copy link
Collaborator

Hmm, can you try creating another copy with this PR? Maybe the GitHub checks are stuck with the stale Travis config.

@lironka
Copy link
Contributor Author

lironka commented Nov 4, 2020

@estherkim Sorry but when I go to https://github.com/ampproject/amphtml/tree/master I see in .codecov.yml the same travis.org. May you get a right master link?

@estherkim
Copy link
Collaborator

Good eye :) that's a bug, but it shouldn't affect the Travis build. Please try creating a fresh PR, thanks!

@lironka
Copy link
Contributor Author

lironka commented Nov 5, 2020

I close this pr due to Travis issue. I reopen it here #31005

@lironka lironka closed this Nov 5, 2020
@lironka lironka deleted the amp-o2-add_consent_data branch November 5, 2020 09:01
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

5 participants