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

chore: Merge Orchestrator history #471

Open
wants to merge 145 commits into
base: main
Choose a base branch
from

Conversation

notlesh
Copy link
Contributor

@notlesh notlesh commented Jan 21, 2025

This PR pulls in the entire madara-orchestrator git history into this project.

Because the entire history was pulled in (using git merge --allow-unrelated-histories) the PR is absolutely massive (and probably difficult to review). My process was basically this:

  1. git merge --allow-unrelated-histories (that is, pull in the entire codebase history)
  2. Move files (crates under crates/orchestrator orchestrator/crates/ and project files to a temp dir)
  3. Update Cargo.toml to build (note that orchestrator is not a member of default-members, disagree if you want!)
  4. Fight dependency conflicts

Status 🚧

So far this builds and madara appears to be unaffected. The project files for madara-orchestrator were kept, but were moved (temporarily) to old_orchestrator_files/. This means that things like CI will not currently work.

TODO:

  • Migrate orchestrator workflows (Orchestrator CI Merge #472)
  • Ensure minimal build and test in github workflows (filter by path, etc.)
  • Update READMEs
  • Test (and fix) orchestrator build/docker/etc
  • Clean up and remove old_orchestrator_files/
  • Remove orchestrator git submodules (This should be trivial, it was previously done)

Repository forks 🍴

This required forking several repositories in order to avoid version conflicts. Each fork uses the same madara-monorepo branch name. These repos were forked:

heemankv and others added 16 commits December 15, 2024 13:42
* update: added verification status to verify job

* update: added verification time

* update: JOB_METADATA_PROCESSING_COMPLETED_AT
* update: added operation_job_status
* chore: readme udpated:

* chore: readme updated

* chore(readme): re-added a few sections that were missed earlier

* chore(readme): linting

* chore: linting and formatting

* chore: linting and formatting

* chore: linting and formatting

* chore: script added for dummy contract deployment, readme udpated to use dummy sharp service

* update: aws_event_bridge rework

* update: e2e

* chore: linting-formatting and readme updated

* chore: readme misinformation fixed

* update: verification job timeout and polling changed to work for 25 hours

* chore: comment added for older python version needed

* refactor: event bridge type added

* chore: linting and formatting of toml files

* update: time delay for txn finality in state update increased to 30 mins

---------

Co-authored-by: mohiiit <[email protected]>
Co-authored-by: Heemank Verma <[email protected]>
* update: fixed block_state collection
* refactor: error handling added for the storage functions

* chore: removed expect

* chore: changelog

* refactor: http-client refactored to handle panic better

---------

Co-authored-by: mohiiit <[email protected]>
Copy link
Collaborator

@Trantorian1 Trantorian1 left a comment

Choose a reason for hiding this comment

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

Given we are moving to a monorepo, I feel like we need to rethink what files we have at the root of the project. We probably want to move any configuration files and READMEs to either crates/madara or crates/orchestrator and keep only common logic at the root.

We also need to think about how we want to run the CI for each project so that this doesn't take for ever on simple changes.

.env.example Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be moved to crates/orchestrator if it is related to that?

.env.test Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

pathfinder Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure we want to strore artefacts under scripts?

Copy link
Collaborator

@Trantorian1 Trantorian1 Jan 22, 2025

Choose a reason for hiding this comment

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

We probably want to figure out a way to compile only SNOS or Madara if we change only one of those no? At least I feel like testing Madara against SNOS should be a separate and last step in the CI if all other steps pass. And make it so that if changes are made to SNOS we do not run e2e on Madara.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like we should tests Madara and SNOS in different CI actions

Copy link
Collaborator

Choose a reason for hiding this comment

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

Imo this should also be under crates/ortchestrator. Given we are making this a monorepo we probably also want to move some Madara-specific files such as the README under crates/madara

@@ -139,7 +153,7 @@ mc-devnet = { path = "crates/madara/client/devnet" }
m-cairo-test-contracts = { path = "crates/madara/cairo-test-contracts" }

# Starknet dependencies
cairo-vm = "=1.0.1"
cairo-vm = { git = "https://github.com/Moonsong-Labs/cairo-vm", branch = "madara-monorepo" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of using forks I feel like we should be importing two separate versions of the same crate under an alias so that madara and the orchestrator can both have their own version. This feels more maintainable long term as far as supporting new features is concerned, and should also make it easier to merge both into a single dependency in the future.

@Trantorian1 Trantorian1 added docs Documentation changes or improvements feature Request for new feature or enhancement infrastructure CI/CD, deployment and infrastructure changes labels Jan 22, 2025
@notlesh notlesh mentioned this pull request Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation changes or improvements feature Request for new feature or enhancement infrastructure CI/CD, deployment and infrastructure changes
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.