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

Remove indexer management GraphQL API #52

Merged

Conversation

Jannis
Copy link
Collaborator

@Jannis Jannis commented Sep 18, 2023

This API is not supposed to be exposed by the indexer service. It is something that only indexer agent runs to give indexer CLI and other tools the ability to configure the indexer agent behavior.

@Jannis Jannis force-pushed the jannis/remove-management-graphql-api branch from 09eef22 to 858e59f Compare September 18, 2023 10:33
@Jannis Jannis changed the title Remove management graphql api Remove management GraphQL api Sep 18, 2023
@Jannis Jannis changed the title Remove management GraphQL api Remove management GraphQL API Sep 18, 2023
@Jannis Jannis changed the title Remove management GraphQL API Remove indexer management GraphQL API Sep 18, 2023
@aasseman aasseman added size:small Small p2 Medium priority type:refactor Changes not visible to users labels Sep 18, 2023
This API is not supposed to be exposed by the indexer service. It is
something that only indexer agent runs to give indexer CLI and other
tools the ability to configure the indexer agent behavior.
Among other things, this makes Emacs happy.
@Jannis Jannis force-pushed the jannis/remove-management-graphql-api branch from 858e59f to ca15834 Compare September 18, 2023 18:52
@Jannis
Copy link
Collaborator Author

Jannis commented Sep 18, 2023

@aasseman @hopeyen Fixed up the linter error, so this should be passing all checks now.

@@ -92,12 +88,6 @@ pub async fn create_server(
get(routes::deployment::deployment_health
.layer(AddExtensionLayer::new(slow_ratelimiter()))),
)
.route(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, good catch, I went too far here. 🤦🏻

I'll put the cost model resolver(s) back.

@Jannis Jannis force-pushed the jannis/remove-management-graphql-api branch from 8b704db to 01dc018 Compare September 18, 2023 19:21
@Jannis Jannis force-pushed the jannis/remove-management-graphql-api branch from 01dc018 to db65564 Compare September 18, 2023 19:24
let pool = &ctx.data_unchecked::<ServerOptions>().indexer_management_db;
indexer_management::cost_model(pool, &deployment).await
}
}

pub(crate) async fn graphql_handler(
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the spirit of remove indexer management graphQL API, perhaps we remove graphql_handler and use a different handler for the cost requests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the public cost model API that indexer-service exposes as well — and that is a GraphQL API. So this is correct.

@Jannis Jannis merged commit cc960b5 into graphprotocol:main Sep 19, 2023
5 checks passed
@Jannis Jannis deleted the jannis/remove-management-graphql-api branch September 19, 2023 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2 Medium priority size:small Small type:refactor Changes not visible to users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants