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(ingestor): validation exception handler #35

Closed
wants to merge 3 commits into from

Conversation

emileten
Copy link
Contributor

@emileten emileten commented Apr 6, 2023

  • add handler for ingestor API invalid request exceptions, so that more informative errors are returned to users of the API
  • modify ingestor tests accordingly
  • make item_assets optional in the Collection schemas.

@emileten emileten changed the title Draft : Handle validation exceptions ingestor feat(ingestor) : validation exception handler Apr 6, 2023
@emileten emileten changed the title feat(ingestor) : validation exception handler feat(ingestor): validation exception handler Apr 6, 2023
@emileten emileten self-assigned this Apr 6, 2023
Copy link
Member

@alukach alukach left a comment

Choose a reason for hiding this comment

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

I am not familiar with why this change is desired. Can you provide more of the backstory?

lib/ingestor-api/runtime/src/schemas.py Show resolved Hide resolved
lib/ingestor-api/runtime/src/main.py Show resolved Hide resolved
@sharkinsspatial
Copy link
Member

With respect to the exception response formatting, I'd tend to agree with @alukach here. In this case, users are not directly parsing the POST response body so I think maintaining a focus on single line returns with a status code that is easily parsable from the logs is the best approach.

With the respect to the item_assets, I'd recommend the following

  • Let's make a separate PR which implements a feature flag configuration to make this validation configurable.
  • When we complete Break out STAC Ingestor codebase into separate repo #4 we had discussed the option of making validation code pluggable so that any consumer application could specify their own validation module that would be injected into the cdk-pgstac construct when the ingestor handler is created. This could be a good first use case for that functionality.

As an aside, can we try to limit the scope of PR's to a functional change. As an example, the title of this PR is feat(ingestor): validation exception handler while the item_assets change is related (as I assume this was the validation error you were hitting and wanted better messages about 😄 ) it makes a bit harder to review and track when we mix functional changes in a single PR.

@emileten
Copy link
Contributor Author

As an aside, can we try to limit the scope of PR's to a functional change. As an example, the title of this PR is feat(ingestor): validation exception handler while the item_assets change is related (as I assume this was the validation error you were hitting and wanted better messages about 😄 ) it makes a bit harder to review and track when we mix functional changes in a single PR.

Yes I agree, but I was hesitant between doing what you say -- split commits based on functionality -- or keep them together to avoid having too many automatic releases happening.

@sharkinsspatial
Copy link
Member

@emileten I hear your pain on the automated releases. Thus far I feel like semantic-release-bot has been a bit to much magic and makes the release process too opaque. If @alukach agrees I think we should consider removing it and moving to a manual tag driven release model.

@alukach
Copy link
Member

alukach commented Apr 12, 2023

I agree that better-scoped PRs are a good thing. Regarding the future of Semantic Release bot, let's work through that on #37

@emileten emileten deleted the handle-validation-exceptions-ingestor branch June 15, 2023 05:46
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.

4 participants