Skip to content

Add executor abstraction #69

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 1 commit into from
Apr 3, 2025
Merged

Add executor abstraction #69

merged 1 commit into from
Apr 3, 2025

Conversation

xinyangli
Copy link
Contributor

@xinyangli xinyangli commented Mar 5, 2025

This PR introduces a framework for CI integration, using Garnix as an example. It's still a proof of concept, though I am currently testing it on my machines.

Changes:

  • Added executor option in config and NixOS module for CI specific options.
  • Implemented evaluation and fetching of Garnix build results with minimal memory usage.
  • Provide possibilities to integrate with other CI.

Known issues:

  • machineId must be exported by CI after evaluation, otherwise we have to evaluate it on target machine. No easy solution found yet.

Related issue: #8

@xinyangli xinyangli marked this pull request as draft March 5, 2025 04:15
@nlewo
Copy link
Owner

nlewo commented Mar 5, 2025

Hello and congrats for this nice POC!

To be honest, i'm a bit afraid by having to maintain this Garnix executor mainly because i don't use Garnix. Moreoever, to test this feature, we would need to mock Garnix API which could evolve.

That being said, i think we could first introduce this executor abstraction without the Garnix part. Then, you could either maintain a fork for few months (in order to be sure you need this feature on the long term) or add an experimental flag when a user wants to enable this Garnix executor.

machineId must be exported by CI after evaluation, otherwise we have to evaluate it on target machine. No easy solution found yet.

Maybe we could add this value in the derivation itself. This would allow comin to read the derivation file to get some values. I don't know the proper way to do this but the comin module could set:

system.systemBuilderArgs.cominJSON = builtins.toJSON {machineId = "my-machine-id";};

The derivation produces by this NixOS configurations can then be used to get back the machineId value:

nix derivation show /nix/store/qn1w3xpsyknj6rmqy2c754jwmnw15sdn-nixos-system.drv | jq 'to_entries[] | .value.env |  .cominJSON'
"{\"machineId\":\"my-machine-id\"}"

Note, I'm planning to get more values from the evaluation (maybe tags/annotations to address this issue).

@xinyangli
Copy link
Contributor Author

Thanks for the feedback! I totally get the maintenance concerns, and I agree—starting with the executor abstraction first makes sense. We can hold off on the Garnix part for now and maybe add an experimental flag later if needed. I will probably propose a more trivial executor later before the Garnix one (like fetching results from a existing file in a branch).

system.systemBuilderArgs.cominJSON = builtins.toJSON { machineId = "my-machine-id"; };

This is a extensible solution! I'll go with this.

Note, I'm planning to get more values from the evaluation (maybe tags/annotations to address this issue.

I also like this idea.

@xinyangli xinyangli force-pushed the main branch 3 times, most recently from a9cf43b to 243b9c1 Compare March 14, 2025 09:09
@xinyangli
Copy link
Contributor Author

Removed Garnix builder for now.

@xinyangli xinyangli marked this pull request as ready for review March 14, 2025 09:14
@xinyangli xinyangli changed the title Integrate with existing CI to avoid evaluation on target Add executor abstraction Mar 14, 2025
Copy link
Owner

@nlewo nlewo left a comment

Choose a reason for hiding this comment

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

It would be really nice if you could open a draft PR with your Garnix executor in order to share it with comin users ;)

@xinyangli xinyangli force-pushed the main branch 2 times, most recently from 8594c8c to bb02d59 Compare April 1, 2025 13:37
@xinyangli xinyangli mentioned this pull request Apr 1, 2025
1 task
@xinyangli
Copy link
Contributor Author

xinyangli commented Apr 1, 2025

Updated accordingly! I've also removed the executor option from golang types. For Garnix executor, see #74

@nlewo nlewo merged commit bca9d37 into nlewo:main Apr 3, 2025
3 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.

2 participants