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

🏗 Enable package updates for E2E and visual diff tests #23427

Merged
merged 2 commits into from
Jul 22, 2019
Merged

🏗 Enable package updates for E2E and visual diff tests #23427

merged 2 commits into from
Jul 22, 2019

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Jul 19, 2019

There's a security vulnerability in one of our test deps that uses an outdated dependency version. It turns out this is because Renovate is configured to upgrade devDependencies, but leave dependencies alone (#23008).

image

This PR moves all of the E2E and visual diff packages to devDependencies to enable automatic upgrades. It also adds some logging to the E2E tests (I found this useful while testing these changes.)

Soon, we should see version upgrade PRs for the packages used by our tests.

@rsimha rsimha self-assigned this Jul 19, 2019
@danielrozenberg
Copy link
Member

Unfortunately we can't upgrade the deps for visual diff that easily, we need to make a significant code change. We could upgrade the lodash dep specifically to resolve this vulnerability (the vulnerability is not that relevant to us, since it allows for code injection in specific instances that we don't encounter - so there' no rush atm)

@rsimha
Copy link
Contributor Author

rsimha commented Jul 19, 2019

Unfortunately we can't upgrade the deps for visual diff that easily, we need to make a significant code change.

I think the Renovate workflow can still be useful for dependency upgrades that don't break our tests, since they will be fully verified on Travis, and are merged manually.

Also, when a new dependency version causes a breaking change (say, with visual tests), there is precedent for closing out the PR and skipping that version.

We could upgrade the lodash dep specifically to resolve this vulnerability

It's an indirect dependency, so it's not trivial to manually upgrade it.

the vulnerability is not that relevant to us, since it allows for code injection in specific instances that we don't encounter - so there' no rush atm

Security vulnerabilities show up on the front page of the amphtml repo (if you have access to view them). I think it's worth addressing them as they appear.

image

@rsimha rsimha requested review from erwinmombay and jridgewell July 19, 2019 22:45
@rsimha
Copy link
Contributor Author

rsimha commented Jul 19, 2019

Adding a couple others for a review, since this is blocking the removal of security vulnerability warnings on the amphtml front page.

@rsimha
Copy link
Contributor Author

rsimha commented Jul 22, 2019

Chatted offline with @danielrozenberg about his concerns about upgrading the visual diff dependencies. This PR will not force-upgrade them. We can close the renovate PRs for @percy/puppeteer and puppeteer until our code is ready for them.

@rsimha rsimha merged commit 6b86000 into ampproject:master Jul 22, 2019
@rsimha rsimha deleted the 2019-07-19-DevDependencies branch July 22, 2019 17:04
@rsimha
Copy link
Contributor Author

rsimha commented Jul 22, 2019

Update: Security alert has been addressed by #23457 which was generated as a result of this PR.

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