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

🏗 Allow renovate to update AMP runtime dependencies #17394

Merged
merged 2 commits into from
Aug 10, 2018
Merged

🏗 Allow renovate to update AMP runtime dependencies #17394

merged 2 commits into from
Aug 10, 2018

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Aug 9, 2018

We've been using tools like Greenkeeper, and now Renovate, to keep the devDependencies section of package.json up to date. However, we've preferred to manually upgrade the dependencies section due to the risk of a breaking change making it in to the AMP runtime.

As it turns out, there's just as much risk in having dependency versions go stale because no one has touched them in years. For example, #15728

This PR enables semi-automatic updates to the dependencies section of package.json. A new PR will be generated for each item on the list that gets an updated. It still falls on the AMP build cop to review the updates, make sure they are safe, and merge them manually.

Update: Per discussion below, we'll only receive updates for dompurify.

@rsimha
Copy link
Contributor Author

rsimha commented Aug 9, 2018

/to @cramforce @choumx @erwinmombay

@dreamofabear
Copy link

What risk was involved in #15728? That particular change needed substantial oversight.

@rsimha
Copy link
Contributor Author

rsimha commented Aug 10, 2018

@choumx The risk I was referring to was when a dependency becomes stale and ends up with a security vulnerability. I might be mistaken about #15728 being such a case. IIRC, we did have a few outdated packages with security vulnerabilities in the past (like #11303), that we had to upgrade.

Do you think it's worth getting an incoming PR when a dependency is updated? Or are we okay with the current status, where they remain static until someone manually checks to see if a new version exists?

@dreamofabear
Copy link

I think we should enable it for dompurify which does have security implications. Not sure about the others. Problems in dev-deps (#11303) are not as serious.

@rsimha
Copy link
Contributor Author

rsimha commented Aug 10, 2018

@choumx Done. PTAL.

@rsimha rsimha merged commit 476240f into ampproject:master Aug 10, 2018
@rsimha rsimha deleted the 2018-08-09-Renovate branch August 10, 2018 21:22
Enriqe pushed a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018
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.

3 participants