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

Add cron job endpoints #63

Merged
merged 5 commits into from
Sep 21, 2023
Merged

Add cron job endpoints #63

merged 5 commits into from
Sep 21, 2023

Conversation

codello
Copy link
Contributor

@codello codello commented Sep 9, 2023

This PR adds endpoints that allow application admins to inspect scheduled background jobs.
The jobs are created by a server admin during application startup but can be inspected and potentially started via the API.

@codello codello added the api Related to the Karman API label Sep 9, 2023
@codello codello added this to the Version 0.1 milestone Sep 9, 2023
@codello codello marked this pull request as ready for review September 9, 2023 21:46
@codello codello enabled auto-merge (squash) September 9, 2023 21:47
Copy link
Collaborator

@kenowi-dev kenowi-dev left a comment

Choose a reason for hiding this comment

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

Since jobs cannot be created or deleted by the API, I am assuming that all job names are known, unique and cannot be changed. If that is the case, I would like to see those names explicitly listed in the API response. Knowing the names beforehand reduces the likelihood, that a user has to run through the keys of the response map.

A more expected response would be a list of jobs, where each job contains its own name.
Since the jobs are referenced by their names, I would expect the name to be a mandatory field in the job-schema and not just a reference key to an unnamed operation.
Also the description of /v1/jobs says Fetch a list of known jobs", which does not indicate a map.

@codello
Copy link
Contributor Author

codello commented Sep 19, 2023

A more expected response would be a list of jobs, where each job contains its own name.

Agreed, updated.

Also the description of /v1/jobs says Fetch a list of known jobs, which does not indicate a map.

Do you think that a list of jobs would make more sense compared to a map? I was thinking that maybe having a map with known keys might be more simple to handle for API clients as a list would introduce some arbitrary order without any meaning. Or would you just change the "list of jobs" into a "map of jobs"? That's what I did in the latest commit just now.

If that is the case, I would like to see those names explicitly listed in the API response.

Could you elaborate on that? I definitely agree that all keys (job names) should be documented in the API spec. Currently the job names are documented in the introduction of the "Background Jobs" section (as it applies to all related endpoints in a way). Would you like to also include all known jobs in the response samples and/or response schemas? Does this only concern the /v1/jobs endpoint or the other endpoints as well?

I think maintaining samples for all available jobs would be quite cumbersome and serve little benefit (especially since the samples would only differ by their names).

@kenowi-dev
Copy link
Collaborator

I think maintaining samples for all available jobs would be quite cumbersome

This is probably true. Having the jobs listed in the "Backgroud Jobs" section, seems like the best choice.

Do you think that a list of jobs would make more sense [...].
Or would you just change the "list of jobs" into a "map of jobs"?

I think a map is fine, if its clear that the keys cannot change and stay fixed. Only their values can change.
I like the newly committed version better, since it specifies, that "all known job" are fetched and not just "a list of".
But to make this even more clear, my suggestion for the /v1/jobs description is: Fetch all "Background Jobs" and their respected states.

@codello
Copy link
Contributor Author

codello commented Sep 21, 2023

Good suggestion. I have added a link to the "Background Jobs" section from both the /v1/jobs and /v1/jobs/{name} endpoints.

@codello codello merged commit 8521ec4 into main Sep 21, 2023
24 checks passed
@codello codello deleted the feature/jobs-api branch September 21, 2023 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to the Karman API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants