Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Allow overriding the complement ref #11766

Merged
merged 3 commits into from
Jan 20, 2022
Merged

Allow overriding the complement ref #11766

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e8a73af
Allow overriding the complement ref
nico-famedly Jan 18, 2022
cedbecf
Ignore all complement directories at the top level
nico-famedly Jan 18, 2022
d689eff
Apply review suggestion
nico-famedly Jan 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/11766.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Allow overriding complement commit using `COMPLEMENT_REF`.
12 changes: 7 additions & 5 deletions scripts-dev/complement.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
# By default the script will fetch the latest Complement master branch and
# run tests with that. This can be overridden to use a custom Complement
# checkout by setting the COMPLEMENT_DIR environment variable to the
# filepath of a local Complement checkout.
# filepath of a local Complement checkout or by setting the COMPLEMENT_REF
# environment variable to pull a different branch or commit.
#
# By default Synapse is run in monolith mode. This can be overridden by
# setting the WORKERS environment variable.
Expand All @@ -31,11 +32,12 @@ cd "$(dirname $0)/.."

# Check for a user-specified Complement checkout
if [[ -z "$COMPLEMENT_DIR" ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious why using COMPLEMENT_DIR won't work for you? Do you not want to pull the entire repo for some reason? (It is relatively small IIRC.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it is easier to update a single environment variable than having to add "install git, clone repo, checkout right commit, set environment variable" to our CI. It's not like we can't do that, but having to enabled/disable that every time the upstream complement doesn't work with the current synapse release is more effort than just telling it to use the right commit. I also just figured out how to do that correctly, after I hacked in the support for specifying a ref and using a ref is less effort. It might also be useful in the future, if complement ever gets branches for synapse versions like "synapse-1.50" or so (because sometimes the branches are just incompatible).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can also easily run the complement job in our CI with the complement ref overwritten, which makes it easier to figure out what commit actually works. We had to do that today, because the complement CA changes somehow don't work in our CI and we couldn't figure out why yet.

Copy link
Member

Choose a reason for hiding this comment

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

Well, it is easier to update a single environment variable than having to add "install git, clone repo, checkout right commit, set environment variable" to our CI.

Boiled down, this PR is the equivalent of adding the listed steps inside of complement.sh. One could also just easily add a "install git, clone repo, checkout right commit, set environment variable" bit to their dockerfile, and control which commit is checked out with another environment variable. And yes, that would work without adding another env var to complement.sh.

But this PR proposes upstreaming it as optional functionality into the script, so that every project doesn't need to set that up their own. It's a convenience that could save downstream developers time, so while it's not a necessary option (and is code we'll need to maintain), I think it's justifiable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's also a really small change, because it just replaces the hardcoded master with an overrideable variable.

Copy link
Member

Choose a reason for hiding this comment

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

It might also be useful in the future, if complement ever gets branches for synapse versions like "synapse-1.50" or so (because sometimes the branches are just incompatible).

We do branch matching in our CI for this reason, see:

synapse/.github/workflows/tests.yml

Lines 342 to 364 in c072c0b

# Attempt to check out the same branch of Complement as the PR. If it
# doesn't exist, fallback to master.
- name: Checkout complement
shell: bash
run: |
mkdir -p complement
# Attempt to use the version of complement which best matches the current
# build. Depending on whether this is a PR or release, etc. we need to
# use different fallbacks.
#
# 1. First check if there's a similarly named branch (GITHUB_HEAD_REF
# for pull requests, otherwise GITHUB_REF).
# 2. Attempt to use the base branch, e.g. when merging into release-vX.Y
# (GITHUB_BASE_REF for pull requests).
# 3. Use the default complement branch ("master").
for BRANCH_NAME in "$GITHUB_HEAD_REF" "$GITHUB_BASE_REF" "${GITHUB_REF#refs/heads/}" "master"; do
# Skip empty branch names and merge commits.
if [[ -z "$BRANCH_NAME" || $BRANCH_NAME =~ ^refs/pull/.* ]]; then
continue
fi
(wget -O - "https://github.com/matrix-org/complement/archive/$BRANCH_NAME.tar.gz" | tar -xz --strip-components=1 -C complement) && break
done

We don't use complement.sh directly for Synapse CI, I'm not sure it is really designed for such things. Anyway, I agree that the overall change is relatively minor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I guess we should do that too at some point then. I was mostly happy we got it working at all :D

COMPLEMENT_REF=${COMPLEMENT_REF:-master}
echo "COMPLEMENT_DIR not set. Fetching the latest Complement checkout..."
nico-famedly marked this conversation as resolved.
Show resolved Hide resolved
wget -Nq https://github.com/matrix-org/complement/archive/master.tar.gz
tar -xzf master.tar.gz
COMPLEMENT_DIR=complement-master
echo "Checkout available at 'complement-master'"
wget -Nq https://github.com/matrix-org/complement/archive/${COMPLEMENT_REF}.tar.gz
tar -xzf ${COMPLEMENT_REF}.tar.gz
COMPLEMENT_DIR=complement-${COMPLEMENT_REF}
nico-famedly marked this conversation as resolved.
Show resolved Hide resolved
echo "Checkout available at 'complement-${COMPLEMENT_REF}'"
fi

# Build the base Synapse image from the local checkout
Expand Down