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

Framework: Remove unused Travis install dependencies #18924

Merged
merged 2 commits into from
Dec 5, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented Dec 4, 2019

Previously: #17004

This pull request seeks to remove the installation step of two seemingly unused dependencies from the Travis configuration install stage. There are no other references to either dotenv or wait-on anywhere in Gutenberg or wordpress-develop, so as best I can tell, they should not be necessary. My assumption is that they were considered in early iterations of #17004 as an alternative to the sleep 10 command which follows a few lines after the removed line.

Testing Instructions:

Travis build should pass.

@aduth aduth added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Code Quality Issues or PRs that relate to code quality labels Dec 4, 2019
@aduth
Copy link
Member Author

aduth commented Dec 4, 2019

There are no other references to either dotenv or wait-on anywhere in Gutenberg or wordpress-develop

Turns out I was searching the built copy of WordPress, not wordpress-develop. There are indeed a few references to each in tools/local-env of the wordpress-develop source we copy a few lines above this in the install step (example).

It's a bit awkward that we satisfy these dependencies this way (vs. installing based on the copied wordpress-develop-master/package.json), but it at least proves to be the case they are indeed required.

I've restored the removed line, but also think it would be wise to include additional comments for future maintainers who find themselves in a similar state of confusion as myself. I've expanded on the existing comment in f609dc6.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

If this is meant to include the code comment after all, it's good to go :)

@aduth
Copy link
Member Author

aduth commented Dec 5, 2019

Yep, only intending to include the clarifying comment after discovering the purpose by which it's needed. Thanks!

@aduth aduth merged commit f2cdea8 into master Dec 5, 2019
@aduth aduth deleted the remove/unused-setup-dependencies branch December 5, 2019 15:59
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants