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

Entrypoint filtering for configuration and deployment APIs #1916

Merged
merged 2 commits into from
Jul 1, 2024

Conversation

mmarchetti
Copy link
Collaborator

Intent

This PR adds a new entrypoint query parameter to the GET /api/configurations and GET /api/deployments endpoints. Only configurations with a matching entrypoint (or deployments whose configuration has a matching entrypoint) will be returned.

Part of #1847

Type of Change

    • Bug Fix
    • New Feature
    • Breaking Change
    • Documentation
    • Refactor
    • Tooling

Automated Tests

Added unit tests specific to the new parameter.

Directions for Reviewers

Test with curl:

$(just executable-path) ui -vv --listen=localhost:9001 &
curl -s 'localhost:9001/api/configurations?dir=test/sample-content/rmd-static-1&entrypoint=static.Rmd'|jq
curl -s 'localhost:9001/api/configurations?dir=test/sample-content/rmd-static-1&entrypoint=index.htm'|jq

Copy link
Collaborator

@dotNomad dotNomad left a comment

Choose a reason for hiding this comment

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

This works great I just had a note about the upcoming recursive work that I was concerned about with this initial implementation. That doesn't matter quite yet though. Tested and this worked with the dir query and without.

Comment on lines +42 to +43
if cfg == nil || cfg.Entrypoint != entrypoint {
continue
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this won't work how I expect when we add recursion. We can adjust when we add that functionality, but the way I would expect this to work is to pass the relative path to the entrypoint and not just simple.py. That way if there are multiple simple.py files in the many folders I get back the only way I care about.

Copy link
Collaborator Author

@mmarchetti mmarchetti Jul 1, 2024

Choose a reason for hiding this comment

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

Yes, we'll need to consider that. I'll make a note in the issue for recursion (#1850).

Copy link
Collaborator

@dotNomad dotNomad left a comment

Choose a reason for hiding this comment

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

Meant to mark this as approved

@mmarchetti mmarchetti merged commit b124372 into main Jul 1, 2024
12 checks passed
@mmarchetti mmarchetti deleted the mm-entrypoint-filtering branch July 1, 2024 22:55
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.

Add entrypoint filtering to and Deployment, Configuration, and Inspect APIs
2 participants