Skip to content

Conversation

@gziolo
Copy link
Member

@gziolo gziolo commented Dec 18, 2018

This PR adds some more debugging details to the shell scripts which installs WordPress instance using Docker.

It also fixes permissions for the files managed on Travis.

@gziolo gziolo added [Type] Bug An existing feature does not function as intended [Type] Build Tooling Issues or PRs related to build tooling labels Dec 18, 2018
@gziolo gziolo added this to the 4.8 milestone Dec 18, 2018
@gziolo gziolo self-assigned this Dec 18, 2018
@gziolo gziolo requested review from a team, aduth, jorgefilipecosta and youknowriad December 18, 2018 14:54
docker-compose $DOCKER_COMPOSE_FILE_OPTIONS run --rm -u 33 $CLI plugin activate gutenberg --quiet

# Install a dummy favicon to avoid 404 errors.
echo -e $(status_message "Intalling a dummy favicon...")
Copy link
Contributor

Choose a reason for hiding this comment

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

typo :)

@gziolo
Copy link
Member Author

gziolo commented Dec 18, 2018

https://wordpress.slack.com/archives/C02QB2JS7/p1545145317102800:

I’m signing off for the day, fix for Travis is ready, can someone take it over and finish the drama present for a few days now? 😄

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

Looks good to me; 🚢 if Travis is green.

I think we need those user ID flags for any command really, I wonder if we should just have a helper that outputs them and our $DOCKER_COMPOSE_FILE_OPTIONS and all that. A thought for a future refactor 😉

},
{
"title": "Contributors",
"title": "Contributors Guide",
Copy link
Member

@Soean Soean Dec 18, 2018

Choose a reason for hiding this comment

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

Should not be here, right?

Suggested change
"title": "Contributors Guide",
"title": "Contributors",

Copy link
Member

Choose a reason for hiding this comment

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

I thought it should be as part of npm run build:docs 🤔

Copy link
Member

@aduth aduth Dec 18, 2018

Choose a reason for hiding this comment

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

See #12910 (comment)

The unreliability of the build has forced people into merging in a failing state. In this case, it was a legitimate failure which was merged.

It must be fixed, so may as well be here, or to master directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

There was no other way to make build pass, if you would open PR with only docs changes introduced it would fail on tests 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

What was the problem here? Did the manifest change at some point but not get committed?

Copy link
Contributor

Choose a reason for hiding this comment

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

(And sorry I'm just seeing this)

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @mkaz who worked on the contributor docs PR

Copy link
Member

Choose a reason for hiding this comment

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

I think I changed the title on the contributors page but did not generate the manifest and commit. We missed it because Travis was throwing false errors, we just assumed it was wrong.

I think ideally we let whatever system that needs the generated file to generate at time of need and not have to try to remember generating and committing.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mkaz do you commit from the command line? I think Husky should have run the check automatically too. If it didn’t that could be another thing to look into.

Copy link
Member

Choose a reason for hiding this comment

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

yep, commit from the command line and didn't see errors there

@gziolo gziolo merged commit 92e3942 into master Dec 18, 2018
@gziolo gziolo deleted the fix/travis-failures branch December 18, 2018 17:20
@tofumatt tofumatt mentioned this pull request Dec 18, 2018
5 tasks
youknowriad pushed a commit that referenced this pull request Jan 9, 2019
* Build tooling: Fix Travis failures with Docker instances

* Docs: Fix manifest file

* chore: Fix typo
youknowriad pushed a commit that referenced this pull request Jan 9, 2019
* Build tooling: Fix Travis failures with Docker instances

* Docs: Fix manifest file

* chore: Fix typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Bug An existing feature does not function as intended [Type] Build Tooling Issues or PRs related to build tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants