Skip to content

docker: project layout updates (bug 1940614)#23

Merged
shtrom merged 33 commits intomainfrom
1940611/pulse-message-update
Apr 2, 2025
Merged

docker: project layout updates (bug 1940614)#23
shtrom merged 33 commits intomainfrom
1940611/pulse-message-update

Conversation

@shtrom
Copy link
Member

@shtrom shtrom commented Feb 4, 2025

Setup and document the project so a standalone syncer can be stood up and manually tested in isolation.

This also adds support for overriding the parameters of the Pulse queue via environment variables, as needed by mozilla-conduit/suite#66.


Looking forwards: more generally, we'll have to override repository and branch configuration per deployment environment. Doing so via env variables will quickly explode. Another option would be to bake (all) per-env configuration files within the source here and into the container image, and select which configuration to use via a single env variable when deploying the container.

This is left for future work, in #24.

@shtrom shtrom force-pushed the 1940611/pulse-message-update branch 2 times, most recently from 8ec0efd to 779e040 Compare February 4, 2025 05:52
Comment on lines +17 to +47
[[tracked_repositories]]
name = "firefox-releases"
url = "/home/fbessou/dev/MOZI/fake-forge/git/firefox-releases"

[[branch_mappings]]
source_url = "/home/fbessou/dev/MOZI/fake-forge/git/firefox-releases"
branch_pattern = "^(esr\\d+)$"
destination_url = "/home/fbessou/dev/MOZI/fake-forge/hg/mozilla-\\1"
destination_branch = "default"

[[tag_mappings]]
source_url = "/home/fbessou/dev/MOZI/fake-forge/git/firefox-releases"
tag_pattern = "^FIREFOX_BETA_(\\d+)_(BASE|END)$"
destination_url = "/home/fbessou/dev/MOZI/fake-forge/hg/mozilla-beta"
tags_destination_branch = "tags"
Copy link
Member Author

@shtrom shtrom Feb 4, 2025

Choose a reason for hiding this comment

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

We'll need to populate something more local at some point.

@shtrom shtrom requested review from cgsheeh, globau and zzzeid February 4, 2025 05:56
cgsheeh
cgsheeh previously approved these changes Feb 4, 2025
Copy link
Member

@cgsheeh cgsheeh left a comment

Choose a reason for hiding this comment

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

Looks great!

@shtrom shtrom force-pushed the 1940611/pulse-message-update branch 4 times, most recently from 3babf19 to 1d72c6e Compare February 5, 2025 05:35
@shtrom shtrom changed the title docker: project layout updates (bug 1940611) docker: project layout updates (bug 1940614) Feb 5, 2025
@shtrom shtrom requested a review from cgsheeh February 7, 2025 00:34
@shtrom
Copy link
Member Author

shtrom commented Feb 7, 2025

@cgsheeh git-hg-sync PR

@shtrom shtrom force-pushed the 1940611/pulse-message-update branch 3 times, most recently from 4db0574 to 831f18a Compare March 17, 2025 03:09
@shtrom shtrom force-pushed the 1940611/pulse-message-update branch 3 times, most recently from e8daffa to 70bc43b Compare March 18, 2025 05:22
@shtrom shtrom marked this pull request as ready for review March 18, 2025 05:55
@shtrom
Copy link
Member Author

shtrom commented Mar 18, 2025

@cgsheeh @zzzeid ok, I think we're good for this one.

zzzeid
zzzeid previously requested changes Mar 21, 2025
[[branch_mappings]]
source_url = "/clones/test-repo-git"
branch_pattern = "beta"
destination_url = "/clones/mozilla-beta" # also, m-c
Copy link
Contributor

Choose a reason for hiding this comment

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

We do have a dev conduit-testing/m-c repo, so would be good to be more explicit about this here. But not sure what this comment means?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hum, I think this was a note-to-self more than anything else. It evolved into #24.

The docker-local test setup uses http://github.com/mozilla-conduit/test-repo as a base, and create all the test repos, so it's not really M-C. My reasoning was that it would be overkill [most of the time] to bandy around multiple copies of M-C.

create_clones.sh Outdated
Comment on lines +31 to +35
# make git status and friends faster:
# mv .git/hooks/fsmonitor-watchman.sample .git/hooks/query-watchman
# git config core.fsmonitor .git/hooks/query-watchman
# add git origin
# git remote add git_chatzilla http://github.com/djangoliv/chatzilla.git
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this code be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. I left the fsmonitor bit as I suspect it could be useful later, but the rest of the comments are no longer needed.

return

if not isinstance(body, dict):
breakpoint()
Copy link
Contributor

Choose a reason for hiding this comment

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

Forgotten breakpoint?

Copy link
Member Author

Choose a reason for hiding this comment

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

😬

try:
if self.event_handler:
self.event_handler(event)
except: # noqa: E722
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there something better here we could do to handle the exception?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really, this is a last-resort catch-all in case we don't know what led to the exception (and therefore how to handle it).

The goal is to make sure we reject the message and re-queue it, rather than dropping it and forgetting all about it, which could lead to inconsistencies in the synced data.

"hashing_algorithm": "rabbit_password_hashing_sha256",
"limits": {},
"name": "guest",
"password_hash": "ywxQjVk0n/qwUfbLIf7L2tJIXrNLfM1bd1GoMeJSzZiRMWSd",
Copy link
Contributor

Choose a reason for hiding this comment

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

How are these values overridden?

Copy link
Member Author

@shtrom shtrom Mar 24, 2025

Choose a reason for hiding this comment

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

RabbitMQ is only here for local testing, so I don't think we need to.

@shtrom shtrom dismissed zzzeid’s stale review March 24, 2025 03:41

Addressed/answered.

@shtrom shtrom requested a review from zzzeid March 24, 2025 03:41
@shtrom shtrom force-pushed the 1940611/pulse-message-update branch from 7a9af4c to b91fe2c Compare March 26, 2025 01:01
@shtrom shtrom requested a review from Copilot March 28, 2025 05:11
@shtrom
Copy link
Member Author

shtrom commented Mar 28, 2025

ok @cgsheeh @zzzeid I think this is good to review for real.

Dockerfile Outdated
RUN pip-compile --verbose pyproject.toml \
&& pip install -r requirements.txt

RUN echo '[ui]\nssh = ssh -oStrictHostKeyChecking=accept-new' >> /etc/mercurial/hgrc \
Copy link
Member

Choose a reason for hiding this comment

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

Nit: we could put the hgrc in a separate file and COPY it into the image.

Comment on lines +47 to +51
# Allow to override Pulse parameters via environment.
for config in self.pulse.model_fields:
env_var = f"PULSE_{config}".upper()
if value := os.getenv(env_var):
setattr(self.pulse, config, value)
Copy link
Member

Choose a reason for hiding this comment

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

We could log updates here, to avoid confusion if it breaks something.

user: str
push_json_url: str

Event = Push
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not rename Push to Event in the class definition?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to retain the ability to handle other events, even if we are down to 1 at the moment.

@shtrom shtrom requested a review from cgsheeh April 1, 2025 05:42
@shtrom shtrom merged commit 388be2c into main Apr 2, 2025
3 checks passed
@shtrom shtrom deleted the 1940611/pulse-message-update branch April 16, 2025 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants