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

feat: add state reconciliation admin cmds #1965

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

oddgrd
Copy link
Contributor

@oddgrd oddgrd commented Jan 17, 2025

Description of change

Add commands to admin CLI for new admin endpoints.

How has this been tested? (if applicable)

Tested in dev environment.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Added new admin CLI commands for managing deployed projects and reconciling project states with ECS.

  • Added DeployedProjects command in admin/src/args.rs to fetch all deployed projects
  • Added ReconcileState command in admin/src/args.rs to reconcile project state with ECS state
  • Potential code duplication between get_all_deployed_projects() and get_projects() in client.rs as they hit same endpoint with different return types

3 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +156 to +159
pub async fn reconcile_state(&self, project_id: &str) -> Result<serde_json::Value> {
let path = format!("/admin/projects/reconcile/{project_id}");
self.inner.post_json(&path, Option::<()>::None).await
}
Copy link

Choose a reason for hiding this comment

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

style: Consider using a more specific return type than serde_json::Value to ensure type safety at compile time

@@ -121,6 +130,10 @@ pub async fn run(args: Args) {
.unwrap();
println!("{res:?}");
}
Command::ReconcileState { project_id } => {
let res = client.reconcile_state(&project_id).await.unwrap();
Copy link

Choose a reason for hiding this comment

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

logic: Using unwrap() here could cause panic if API call fails. Consider adding error context like other commands do with expect()

Comment on lines +152 to +154
let path = format!("/admin/projects");
self.inner.get_json(&path).await
}
Copy link

Choose a reason for hiding this comment

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

style: format! is unnecessary here since no string interpolation is being done

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.

1 participant