Skip to content

Feat: bundle builds #507

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

Merged
merged 13 commits into from
Mar 19, 2025
Merged

Feat: bundle builds #507

merged 13 commits into from
Mar 19, 2025

Conversation

SRWieZ
Copy link
Member

@SRWieZ SRWieZ commented Mar 4, 2025

No description provided.

@simonhamp simonhamp added this to the v1 milestone Mar 13, 2025
@SRWieZ SRWieZ marked this pull request as ready for review March 14, 2025 09:38
@simonhamp
Copy link
Member

Something is making all of the tests crash here. Not sure what

@simonhamp
Copy link
Member

simonhamp commented Mar 18, 2025

I've superficially fixed the tests here... I think the changes in that commit highlight some things about the implementations and the changes that have crept in.

I think it's fine for now, but I'm a little concerned around the QueueWorker alias stuff... the complexity there seems to be that we have an alias for the ChildProcess itself (which seems to have grown a queue_ prefix) and a --name for the queue worker.

It looks like these aliases were identical before, but that perhaps you tried to disambiguate?

I'm fine for it to be in its current state, but we should rationalise the ideal state and settle on it now before v1. My concern is that neither approach is perfect and that in either case we will need something in the docs to explain to devs what's happening.

@PeteBishwhip
Copy link
Member

Dropped in a quick little merge conflict resolution commit.

@SRWieZ
Copy link
Member Author

SRWieZ commented Mar 19, 2025

I've superficially fixed the tests here... I think the changes in that commit highlight some things about the implementations and the changes that have crept in.

I think it's fine for now, but I'm a little concerned around the QueueWorker alias stuff... the complexity there seems to be that we have an alias for the ChildProcess itself (which seems to have grown a queue_ prefix) and a --name for the queue worker.

It looks like these aliases were identical before, but that perhaps you tried to disambiguate?

Yes.

Because, by default, a child process named default was launched and shown in the console/logs, but a newcomer would not understand that it's the queue worker that has been launched.

I made this change to separate queue workers from other child processes and avoid conflicting names. The name of the queue didn't change, but the alias on the child process has a prefix now.

I looked at and approved your test fix. They correctly reflects my changes.

I'm fine for it to be in its current state, but we should rationalise the ideal state and settle on it now before v1. My concern is that neither approach is perfect and that in either case we will need something in the docs to explain to devs what's happening.

Yep.

@simonhamp simonhamp merged commit a8b0a72 into main Mar 19, 2025
44 checks passed
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.

3 participants