-
-
Notifications
You must be signed in to change notification settings - Fork 828
Conversation
ea26043
to
378bf68
Compare
69a4df9
to
cab5f7a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking reasonable. Again I have some general questions to get things moving.
Nit: Buildkite seems to spell their name with lowercase "K".
# TODO: Remove hacky chmod for BuildKite | ||
- "chmod +x ./scripts/ci/*.sh" | ||
- "chmod +x ./scripts/*" | ||
- "./scripts/ci/install-deps.sh" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still feels wasteful to duplicate so much of the installation... I can't tell from the BK log how long each step takes right now, since the log get truncated...
Speaking of that, the biggest factor in our log length seems to be Webpack's --progress
output. Can we change Webpack options used in CI so that the log shows up start to finish in BK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The log gets truncated for the web view only - as mentioned in the error, there's a download button for when your build explodes and you want to see the whole log. Historically, our stuff breaks before it would reach the point of truncation.
@@ -135,9 +135,10 @@ module.exports = function (config) { | |||
], | |||
|
|||
customLaunchers: { | |||
'ChromeHeadless': { | |||
'VectorChromeHeadless': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still use the Vector
prefix that much, now that it's called Riot? Also, what's the purpose of the custom name here in any case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using Vector in the company sense here, not in the product sense. ChromeHeadless
is an actual browser version and seems to confuse Buildkite (but not Travis CI for some reason), so I've essentially named it New Vector's ChromeHeadless Browser v10: Spectacular feature release for CI environments
without using so many words.
Gives the build the best possible chance at passing
scripts/ci/unit-tests.sh
Outdated
@@ -7,4 +7,3 @@ | |||
set -ev | |||
|
|||
scripts/ci/build.sh | |||
CHROME_BIN='/usr/bin/google-chrome-stable' yarn test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, I think you only wanted to delete the CHROME_BIN
env var here... 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naaaaaaaaaahhhhhh....
(oops)
clone $TRAVIS_BRANCH | ||
clone $BUILDKITE_BRANCH | ||
# Try the PR author's branch in case it exists on the deps as well. | ||
clone $BUILDKITE_PULL_REQUEST_BASE_BRANCH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bringing these back. The clone
lines seem correct and are in the correct order, but the comments are mismatched ($BUILDKITE_BRANCH
is the PR author's branch), so we should swap the comments. I believe Riot Web's fetch-develop.deps.sh
has the right ordering for both code and comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, this looks good to me! 😁 Thanks again for working on this!
TODO: Better pipelines (see element-hq/element-web#9019)
Part of fixing element-hq/element-web#9164
Partnered with: