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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃彈 Ensure that analytics-vendors-list.md is up to date #32855

Merged
merged 19 commits into from Feb 25, 2021
Merged

馃彈 Ensure that analytics-vendors-list.md is up to date #32855

merged 19 commits into from Feb 25, 2021

Conversation

alanorozco
Copy link
Member 

git check-analytics-vendors-list --fix

[20:37:39] Using gulpfile ~/git/amphtml/gulpfile.js
[20:37:39] Starting 'check-analytics-vendors-list'...

diff --git a/extensions/amp-analytics/analytics-vendors-list.md b/extensions/amp-analytics/analytics-vendors-list.md
index 31bdce4b3..8c8332077 100644
--- a/extensions/amp-analytics/analytics-vendors-list.md
+++ b/extensions/amp-analytics/analytics-vendors-list.md
@@ -110,2 +110,8 @@ Adds support for Clicky Web Analytics. More details for adding Clicky support ca
 
+### colanalytics
+
+Type attribute value: `colanalytics`
+
+DO NOT SUBMIT: Add a paragraph to describe colanalytics.
+
 ### comScore
@@ -176,2 +182,8 @@ Adds support for Google Analytics. More details for adding Google Analytics supp
 
+### gtag
+
+Type attribute value: `gtag`
+
+DO NOT SUBMIT: Add a paragraph to describe gtag.
+
 ### Google Tag Manager
@@ -203,2 +215,8 @@ More d

[20:37:39] Wrote extensions/amp-analytics/analytics-vendors-list.md
[20:37:39] Finished 'check-analytics-vendors-list' after 92 ms

alanorozco
alanorozco commented Feb 24, 2021
View reviewed changes
extensions/amp-analytics/analytics-vendors-list.md Outdated

Adds support for Webtrekk. Configuration details can be found at [supportcenter.webtrekk.com](https://supportcenter.webtrekk.com/en/public/amp-analytics.html).
> 鈿狅笍 ~~`webtrekk`~~ is deprecated. **Use `webtrekk_v2` instead.**
Copy link
Member Author

Choose a reason for hiding this comment

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

Previously:

The attribute value webtrekk is deprecated (will remove on 31/12/2018) - use webtrekk_v2 instead

@micajuine-ho

Do we have a process for removing vendor types in general? Can we remove this one in particular?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe we do. I would imagine we don't since it would be breaking if someone were to use it and a simple change in the analytics code (or cache) to change type=webtrekk to type=webtrek_v2 might not work b/c inline configs could be incompatible.

We could do assess how often webtrekk (not v2) is used via cookbook and then evaluate how much of an impact deprecation would have.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we don't have a deprecation process, we don't need to create one. The blurb should be enough.

alanorozco
alanorozco commented Feb 24, 2021
View reviewed changes
extensions/amp-analytics/analytics-vendors-list.md Outdated
Comment on lines 82 to 92
### bg.canary

Type attribute value: `bg.canary`

DO NOT SUBMIT: Add a paragraph to describe bg.canary.

### bg

Type attribute value: `bg`

DO NOT SUBMIT: Add a paragraph to describe bg.
Copy link
Member Author

Choose a reason for hiding this comment

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

@micajuine-ho Are these relevant?

Copy link
Contributor

Choose a reason for hiding this comment

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

bg and bg.canary are special cases the iframe transport situation. They were added in this PR #11712 but I don't have any insight to who they are.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll comment these out. If they face the user somehow we might need to bring them back.

@alanorozco alanorozco marked this pull request as ready for review February 24, 2021 16:06
@amp-owners-bot
Copy link

Hey @rsimha! These files were changed:

build-system/pr-check/checks.js

rsimha
rsimha approved these changes Feb 24, 2021
View reviewed changes
Copy link
Contributor

@rsimha rsimha left a comment •
edited

Choose a reason for hiding this comment

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

We're on a roll here!

@micajuine-ho
Copy link
Contributor

@alanorozco What does the gulp task do? Does it ensure that all analytics vendor types have an associated description in vendor-list.md?

@alanorozco
Copy link
Member Author

@micajuine-ho Correct. It will block CI, but can be used to update locally: git check-analytics-vendors-list --fix

@alanorozco alanorozco merged commit 60ec06c into ampproject:master Feb 25, 2021
@alanorozco alanorozco deleted the check-analytics-vendors-list branch February 25, 2021 16:17
@alanorozco alanorozco mentioned this pull request Mar 8, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rsimha rsimha rsimha approved these changes

@micajuine-ho micajuine-ho micajuine-ho approved these changes

Assignees
No one assigned
Labels
PR use: In Beta / Experimental PR use: In Stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@alanorozco @micajuine-ho @rsimha @ampprojectbot