Skip to content
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

temporal: add temporal service #633

Merged
merged 3 commits into from
Jun 18, 2023

Conversation

sagikazarmark
Copy link
Contributor

@sagikazarmark sagikazarmark commented May 31, 2023

Fixes #632

@sagikazarmark sagikazarmark force-pushed the add-temporal-service branch 3 times, most recently from f3ad4a8 to 5248354 Compare May 31, 2023 11:42
@sagikazarmark sagikazarmark marked this pull request as ready for review May 31, 2023 11:42
@sagikazarmark
Copy link
Contributor Author

I'm open to feedback, particularly about the ephemeral option. Not sure that's the right name/structure, especially since there are other state related parameters (eg. sqlite-pragma) that might make sense to be exposed.

@sagikazarmark sagikazarmark force-pushed the add-temporal-service branch from 5248354 to 539c18b Compare May 31, 2023 11:45
Signed-off-by: Mark Sagi-Kazar <mark.sagikazar@gmail.com>
@sagikazarmark sagikazarmark force-pushed the add-temporal-service branch from 539c18b to 26c4411 Compare May 31, 2023 12:05
@sagikazarmark
Copy link
Contributor Author

sagikazarmark commented May 31, 2023

Would this be better compared to the current ephemeral option?

{
    state = {
        ephemeral = true|false;
        sqlite-pragma = {
            # ...
        };
    };
}

@sagikazarmark
Copy link
Contributor Author

I added a separate section for state config matching the above.

@sagikazarmark
Copy link
Contributor Author

Hm, that bind: address already in use error is weird. Any chance these tests run in parallel on those machines? Would it make sense to randomize ports?

Maybe it just needs a retry.

@shyim
Copy link
Contributor

shyim commented Jun 2, 2023

Does temporal run in foreground so it can be properly stopped?

@sagikazarmark
Copy link
Contributor Author

sagikazarmark commented Jun 2, 2023

It does, but the shutdown process may not be instantaneous.

@sagikazarmark sagikazarmark force-pushed the add-temporal-service branch from f9d5bea to 55c4666 Compare June 5, 2023 08:25
Signed-off-by: Mark Sagi-Kazar <mark.sagikazar@gmail.com>
@sagikazarmark sagikazarmark force-pushed the add-temporal-service branch from 55c4666 to 525895f Compare June 7, 2023 09:33
@sagikazarmark
Copy link
Contributor Author

I tried changing the port, maybe there is a collision.

@sagikazarmark sagikazarmark force-pushed the add-temporal-service branch from 673f55f to 4f2b5db Compare June 7, 2023 23:24
@sagikazarmark
Copy link
Contributor Author

Apparently, there is something running on that port. Changing the port fixes the problem. I also fixed the script, so hopefully it's going to work now.

Signed-off-by: Mark Sagi-Kazar <mark.sagikazar@gmail.com>
@sagikazarmark sagikazarmark force-pushed the add-temporal-service branch from 4f2b5db to 1fc5976 Compare June 8, 2023 12:51
@sagikazarmark
Copy link
Contributor Author

Breaking jobs seem unrelated to me. I think this is ready.

@shyim
Copy link
Contributor

shyim commented Jun 8, 2023

seems related to NixOS/nixpkgs#233265

@sagikazarmark
Copy link
Contributor Author

Can I get a review on this? Since the breaking builds are unrelated, any chance this can get merged?

@domenkozar domenkozar merged commit 9284e05 into cachix:main Jun 18, 2023
@sagikazarmark sagikazarmark deleted the add-temporal-service branch June 19, 2023 07:44
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.

Add Temporal process
4 participants