Skip to content

Add AdSpeed ad server to amp-ad#7219

Merged
lannka merged 5 commits intoampproject:masterfrom
Adspeed:master
Feb 15, 2017
Merged

Add AdSpeed ad server to amp-ad#7219
lannka merged 5 commits intoampproject:masterfrom
Adspeed:master

Conversation

@Adspeed
Copy link
Copy Markdown
Contributor

@Adspeed Adspeed commented Jan 26, 2017

Hello,

I've followed the guidelines and examples to add support for AdSpeed ad server into amp-ad.

  • No error with gulp lint
  • Ran & passed tests "test-ads-config.js,test-integration.js"
  • Tested locally with "examples/ads.amp.max.html?type=adspeed"
  • Signed Google CLA (git username: "adspeed")

Please review and let me know. Thanks!

@jridgewell
Copy link
Copy Markdown
Contributor

/to @lannka

@lannka lannka self-assigned this Feb 1, 2017

adspeed: {
preconnect: 'https://g.adspeed.net',
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could you also implement the render-start and no-content API?

@Adspeed
Copy link
Copy Markdown
Contributor Author

Adspeed commented Feb 2, 2017

Hello, I've just updated our backend JS code to call the API renderStart() and noContentAvailable() when suitable and I modified ads/_config.js. However, Travis CI build failed (error below), which I don't think is because of our code, looks like a config thing. Please check and restart build if it's the case. Thanks

"The command "node build-system/pr-check.js" exited with 1."

@Adspeed
Copy link
Copy Markdown
Contributor Author

Adspeed commented Feb 7, 2017

Hello. Can someone help restart Travis build? Thanks!

@jridgewell
Copy link
Copy Markdown
Contributor

Restarted.

@Adspeed
Copy link
Copy Markdown
Contributor Author

Adspeed commented Feb 7, 2017

It failed very early again at: "pr-check.js - any update to package.json or yarn.lock must include the other file. Please update through yarn."

Anyone knows why?

I re-ran these local checks on my machine without any issue:

  • gulp lint
  • gulp check-types
  • gulp build
  • gulp serve

Thanks for your help

@jridgewell
Copy link
Copy Markdown
Contributor

/cc @erwinmombay

@Adspeed
Copy link
Copy Markdown
Contributor Author

Adspeed commented Feb 13, 2017

Hello,

Any help with the build config/troubleshooting/result?

Or should I close this PR and restart using the latest amphtml revision?

Thanks!

@jridgewell
Copy link
Copy Markdown
Contributor

Try rebasing against current master? Assuming you called ampproject/amphtml the upstream remote:

$ git pull --rebase upstream/master

@Adspeed
Copy link
Copy Markdown
Contributor Author

Adspeed commented Feb 14, 2017

Thanks Justin! That (updating/syncing the fork) definitely helped and all checks have passed now. Please review.

@lannka lannka merged commit 732b5fe into ampproject:master Feb 15, 2017
@Adspeed
Copy link
Copy Markdown
Contributor Author

Adspeed commented Feb 15, 2017

Thanks!!

mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
* Add AdSpeed ad server to amp-ad

* Change zid/oid to zone/client since oid is conflicting

* Implemented renderStart() and noContentAvailable() API in AdSpeed backend. Tested both scenarios.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants