♻️ Conformance check allowlist by kristoferbaxter · Pull Request #28778 · ampproject/amphtml

kristoferbaxter · 2020-06-09T19:44:13Z

Enforce usage of allowlist instead of other terms.

build-system/tasks/presubmit-checks.js

amp-owners-bot · 2020-06-09T20:09:48Z

Hey @jeffkaufman! These files were changed:

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

Hey @gmajoulet, @newmuis, @Enriqe! These files were changed:

extensions/amp-story/0.1/amp-story-consent.js
extensions/amp-story/0.1/amp-story-grid-layer.js
extensions/amp-story/0.1/amp-story.js
extensions/amp-story/0.1/origin-allowlist.js
extensions/amp-story/0.1/test/test-amp-story-consent.js
extensions/amp-story/0.1/test/test-amp-story.js
extensions/amp-story/0.1/test/test-origin-allowlist.js
extensions/amp-story/1.0/amp-story-access.js
extensions/amp-story/1.0/amp-story-consent.js
extensions/amp-story/1.0/amp-story-grid-layer.js
extensions/amp-story/1.0/amp-story-store-service.js
extensions/amp-story/1.0/amp-story.js
extensions/amp-story/1.0/test/test-amp-story-access.js
extensions/amp-story/1.0/test/test-amp-story-consent.js
extensions/amp-story/1.0/test/test-amp-story-store-service.js
extensions/amp-story/1.0/test/test-amp-story.js
extensions/amp-story/validator-amp-story.protoascii

Hey @ampproject/wg-caching! These files were changed:

validator/engine/htmlparser.js
validator/engine/validator.js
validator/engine/validator_test.js
validator/java/src/main/java/dev/amp/validator/utils/TagSpecUtils.java
validator/java/src/main/proto/validator.proto
validator/testdata/feature_tests/ads.html
validator/testdata/feature_tests/ads.out
validator/testdata/feature_tests/mandatory_dimensions.html
validator/testdata/feature_tests/mandatory_dimensions.out
validator/testdata/feature_tests/no_custom_js.html
validator/testdata/feature_tests/no_custom_js.out
validator/validator-css.protoascii
validator/validator-main.protoascii
validator/validator.pb.go
validator/validator.proto

jridgewell

Everything else LGTM.

jridgewell · 2020-06-09T20:19:59Z

extensions/amp-story/1.0/amp-story-store-service.js

 /** @const @enum {string} */
 export const Action = {
-  ADD_TO_ACTIONS_WHITELIST: 'addToActionsWhitelist',
+  ADD_TO_ACTIONS_ALLOWLIST: 'addToActionsAllowlist',


Changes to this file need @ampproject/wg-stories approval. I can't verify if this enum string is leaked to external code, or if this is truly internal.

Adding a @ampproject/wg-stories member (@gmajoulet) to review.

caroqliu

LGTM, but needs a global replace for "a allowlist" to "an allowlist"

caroqliu · 2020-06-09T20:32:12Z

test/unit/test-batched-json.js

    urlReplacements = {
      expandUrlAsync: window.sandbox.stub(),
-      collectUnwhitelistedVarsSync: window.sandbox.stub(),
+      collectUnallowlistedVarsSync: window.sandbox.stub(),


Doesn't have to be addressed now but it's worth noting there is some inconsistency throughout with regards to "unallowlisted" versus "nonallowlisted"

lgtm-com · 2020-06-09T22:55:21Z

This pull request introduces 1 alert when merging c1df0ad into 2121908 - view on LGTM.com

new alerts:

1 for Syntax error

gmajoulet

Story code LGTM.

ads/google/utils.js

gmajoulet · 2020-06-10T01:10:57Z

extensions/amp-a4a/rtc-documentation.md


 The AMP-Consent extension provides publishers the ability to collect and store a user's consent through a UI control, while also providing the ability to block other AMP components based on the user's consent. See [here for documentation](https://github.com/ampproject/amphtml/blob/master/extensions/amp-consent/amp-consent.md).

+<<<<<<< HEAD


Here and below: merge conflicts

calebcordry

Ads LGTM.

calebcordry · 2020-06-10T16:17:15Z

src/service/url-replacements-impl.js

   */
-  expandSyncIfAllowList_(href, allowList) {
-    return allowList
+  expandSyncIfAllowedList_(href, allowlist) {


nit: I think we are using allowlisted elsewhere.

samouri · 2020-06-10T16:31:31Z

extensions/amp-animation/0.1/web-animation-types.js


 /** @const {!Object<string, boolean>} */
-const WHITELISTED_RPOPS = {
+const ALLOWLISTED_RPOPS = {


ahhhhh RPOPSSSSSSSSSS --> PROPS

sorry for the driveby but this typo had me chuckling 😂

That's amazing, absolutely didnt notice.

Disallow older terms for allowlist

b796410

kristoferbaxter requested review from caroqliu and rsimha and removed request for rsimha June 9, 2020 19:44

googlebot added the cla: yes label Jun 9, 2020

kristoferbaxter requested review from Gregable, calebcordry, cramforce and rsimha June 9, 2020 19:44

rsimha reviewed Jun 9, 2020

View reviewed changes

build-system/tasks/presubmit-checks.js Show resolved Hide resolved

rsimha approved these changes Jun 9, 2020

View reviewed changes

jridgewell reviewed Jun 9, 2020

View reviewed changes

Gregable approved these changes Jun 9, 2020

View reviewed changes

kristoferbaxter requested a review from gmajoulet June 9, 2020 20:31

caroqliu approved these changes Jun 9, 2020

View reviewed changes

Merge latest master

c1df0ad

gmajoulet approved these changes Jun 10, 2020

View reviewed changes

kristoferbaxter added 4 commits June 10, 2020 08:14

Conflict

78f13b6

rtc documentation conflict

ff9a0d6

Pull in latest master

f01fc3f

disallowedVarsSync

5cc0a37

calebcordry approved these changes Jun 10, 2020

View reviewed changes

merge conflict with master

839c573

samouri reviewed Jun 10, 2020

View reviewed changes

cramforce approved these changes Jun 10, 2020

View reviewed changes

kristoferbaxter merged commit feb792f into ampproject:master Jun 10, 2020

kristoferbaxter deleted the conformance-allowlist branch June 10, 2020 17:24

ampprojectbot added the PR use: In Beta / Experimental label Jun 18, 2020


		The AMP-Consent extension provides publishers the ability to collect and store a user's consent through a UI control, while also providing the ability to block other AMP components based on the user's consent. See [here for documentation](https://github.com/ampproject/amphtml/blob/master/extensions/amp-consent/amp-consent.md).

		<<<<<<< HEAD

Conversation

kristoferbaxter commented Jun 9, 2020

Uh oh!

Uh oh!

amp-owners-bot bot commented Jun 9, 2020 • edited by ampprojectbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jridgewell left a comment

Choose a reason for hiding this comment

Uh oh!

jridgewell Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

kristoferbaxter Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caroqliu left a comment

Choose a reason for hiding this comment

Uh oh!

caroqliu Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Jun 9, 2020

Uh oh!

gmajoulet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmajoulet Jun 10, 2020

Choose a reason for hiding this comment

Uh oh!

calebcordry left a comment

Choose a reason for hiding this comment

Uh oh!

calebcordry Jun 10, 2020

Choose a reason for hiding this comment

Uh oh!

samouri Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kristoferbaxter Jun 10, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

amp-owners-bot bot commented Jun 9, 2020 •

edited by ampprojectbot

Loading

kristoferbaxter Jun 9, 2020 •

edited

Loading

samouri Jun 10, 2020 •

edited

Loading