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

FIX: Update to work with the rewritten API #38

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

Conversation

kamil-certat
Copy link

The small adjustments were needed to pass tests
with the API rewritten to the FastAPI. In addition,
missing debconf configuration was added.

The small adjustments were needed to pass tests
with the API rewritten to the FastAPI. In addition,
missing debconf configuration was added.
@kamil-certat
Copy link
Author

@sebix @aaronkaplan Please take a look for testing FastAPI with the playbook

@@ -5,4 +5,4 @@
register: queuesnoauth
- name: Check if authentication works
assert:
that: "'{\"errors\": {\"Authentication Required\": \"Please provide valid Token verification credentials\"}}' in queuesnoauth.content"
that: "'{\"error\":{\"Authentication Required\":\"Please provide valid Token verification credentials\"}}' in queuesnoauth.content"
Copy link
Member

Choose a reason for hiding this comment

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

If the response changed, that indicates a breaking change.

Copy link
Author

Choose a reason for hiding this comment

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

This is unfortunately true, but I didn't find any troubles with the IntelMQ Manager so far. But it needs to be verified one more time.

There are unfortunately some disadvantages of how hug works:

  1. the errors key in messages on auth error were set by hug; the rest of the API is using error (I assume the idea was to follow the same key, but it failed);
  2. hug was returning the proper JSON response, but... without setting the proper content-type header. IntelMQ Manager handles well both versions.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for checking all the details! I agree it's OK to go with the change then, but it should be mentioned in the CHANGELOG of intelmq-api

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.

2 participants