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

Implemented views with django rest framework and serializers #295

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

JakobJBauer
Copy link

No description provided.

@maszaa
Copy link
Contributor

maszaa commented Apr 22, 2021

Would JSON response include time_taken for each plugin?
If yes, this would implement #296

@JakobJBauer
Copy link
Author

Would JSON response include time_taken for each plugin?
If yes, this would implement #296

Yes it does, but I also changed the JSON-Response into a more machine-readable way.
It now is

{
    "data": [
        {
            "name": "foo",
            "status": "working",
            "time_taken": 0.001123
        },
        {
            "name": "bar",
            "status": "working",
            "time_taken": 1.23353e-05
        },
        {
            "name": "foobar",
            "status": "failed",
            "time_taken": 0.501123
        }
    ]
}

The beauty of using serializers is, that you get automatic documentation on your API endpoints. That way it fits the specification of OpenAPI better.

I am currently fixing the behavior so that no tests fail because of the changed JSON response, but locally everything works fine. I will mark this pull request as Ready for review as soon as all tests go through
The HTML-Response is unchanged

@JakobJBauer
Copy link
Author

All test were successful on my forked repository.

@JakobJBauer JakobJBauer marked this pull request as ready for review April 23, 2021 12:22
@JakobJBauer JakobJBauer changed the title WIP: Implemented views with django rest framework and serializers Implemented views with django rest framework and serializers Apr 23, 2021
@maszaa
Copy link
Contributor

maszaa commented Apr 24, 2021

I like the DRF and serializer approach. Good job!

But the change to JSON output is a breaking change as the format change is backwards incompatible. How about versioning the API? It could be a query parameter, a header etc. That would allow current users to update the package and their integrations would continue to work without modifications.

Currently the JSON output is generated if format=json query parameter is present. It seems to be controlled via Accept header after this change. That's also a breaking change.

.gitignore Outdated Show resolved Hide resolved
health_check/health_serializer.py Outdated Show resolved Hide resolved
health_check/views.py Outdated Show resolved Hide resolved
tests/test_views.py Outdated Show resolved Hide resolved
@JakobJBauer
Copy link
Author

JakobJBauer commented Apr 27, 2021

I pushed a new Version with the old tests and API, as well as the serializers approach as v2. It can be addressed by adding /v2 to the end of the URL.

However, I can into a problem that I couldn't resolve. Two tests fail (test_success and test_warning) if both, the old testfile (test_views_v1.py) and the new testfile (test_views_v2.py) are tested.
If one of the test files is removed, the other one runs without any issues. It seems to be a problem with client or some sort of instance that doesn't get reloaded in between. It is also always the second test file that fails, so if you change the order of execution, the first runs through (regardless of v1 or v2), but the second one has two errors. Maybe you can suggest what could be the issue here?

Now everything should be backwards compatibel.

@maszaa
Copy link
Contributor

maszaa commented Apr 27, 2021

However, I can into a problem that I couldn't resolve. Two tests fail (test_success and test_warning) if both, the old testfile (test_views_v1.py) and the new testfile (test_views_v2.py) are tested.
If one of the test files is removed, the other one runs without any issues. It seems to be a problem with client or some sort of instance that doesn't get reloaded in between. It is also always the second test file that fails, so if you change the order of execution, the first runs through (regardless of v1 or v2), but the second one has two errors. Maybe you can suggest what could be the issue here?

The issue is that both v1 and v2 tests reset plugin_dir and modify HEALTH_CHECK of conf.py but don't restore those after the test suites.

Adding following to TestMainView and TestDRFView fixes the issue:

@classmethod
def setup_class(cls):
    cls.registry = plugin_dir._registry.copy()
    HEALTH_CHECK['WARNINGS_AS_ERRORS'] = True
        
@classmethod
def teardown_class(cls):
    plugin_dir._registry = cls.registry.copy()
    HEALTH_CHECK['WARNINGS_AS_ERRORS'] = True

@JakobJBauer
Copy link
Author

Thank you, works like a charm!

I need workflow approval to run the tests here, after that everything should be ready to merge (identical Tests went well on my own repository)

@maszaa
Copy link
Contributor

maszaa commented Apr 28, 2021

@codingjoe Could you approve the workflow? The PR looks good to me.

@JakobJBauer
Copy link
Author

@maszaa @codingjoe Is this PR still considered?

@codingjoe
Copy link
Collaborator

Hi @JakobJBauer,

Thanks for reaching out. To be honest, I am looking for a new maintainer, so I might be a bit slower than usual to respond here.
Anyhow, before I start reviewing your PR, would you mind explaining your intentions here a bit? It doesn't seem to add any new functionalities, or am I missing something?

Best,
Joe

@JakobJBauer
Copy link
Author

Hello @codingjoe
my PR adds a v2 for the API, to be REST-compliant.
It added "time taken" to the JSON Response (before, you could only get that via html) and was done by using serializers, which allows other frameworks, to automatically build the REST-documentation without any additional input.

The old version is still the default and all tests went through, so there should be no issues regarding backward compatibility. Also, there is no new required dependency, as it only imports new librarys if they are already installed (and enables v2 then).

The new version can be addressed by adding 'v2/' into the API request (described in the README.md)

@JakobJBauer
Copy link
Author

JakobJBauer commented Jul 23, 2021

Hi @JakobJBauer,

Thanks for reaching out. To be honest, I am looking for a new maintainer, so I might be a bit slower than usual to respond here.
Anyhow, before I start reviewing your PR, would you mind explaining your intentions here a bit? It doesn't seem to add any new functionalities, or am I missing something?

Best,
Joe

If you are looking for a new maintainer, maybe JazzBand would take care of this project. They are a group of people that maintain many different projects, but most of them are related to django, so they should already be familiar with most of this project.
Here are some guidelines to transfer a project to jazzband.

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.

3 participants