Add readonly connection endpoints #9095
Conversation
ephraimbuddy
changed the title
ephraimbuddy
changed the title
There was a problem hiding this comment.
Great idea. I wonder if pagination is handled here correctly? When we have 100 entries in the database, the user will fetch only two entries (limit = 2), then total_entires should still be 100.
There was a problem hiding this comment.
I have not tried pagination. Let me write a testcase for it
There was a problem hiding this comment.
It works on pagination. I will push now. Please, Is there a way I can change this PR back to draft?
There was a problem hiding this comment.
I think we should add nullable to the specification in a few places. This was not done early because we focused on the general API specification.
There was a problem hiding this comment.
That would be great,
There was a problem hiding this comment.
It seems to me that we could do this by extending the fields.String or fields.Int class. It will be clearer.
There was a problem hiding this comment.
Good. I will try it
There was a problem hiding this comment.
Hi, I was not able to fix this by extending fields but adding nullable to the specification like you said fixed it. This is the fix 819e036
Also I am thinking that the spec changes can be done in a separate PR
There was a problem hiding this comment.
Also I am thinking that the spec changes can be done in a separate PR
You can suggest it. For me, it's okay if the changes are proposed with context.
There was a problem hiding this comment.
Can you add support for 404 as well?
ephraimbuddy
changed the title
|
A question. Should we have a directory/filename with package name in it ? Like here we have airflow/api_connexion/endpoints/connection_endpoint.py. This path contains connexion name in it. IMHO code should not reflect underlying package. Just my opinion. |
|
@vbmade2000 Packages should clearly show the content. We already have the api package, so I had to use another package that best describes its content. api_connexion is best in my opinion, because we rely on connexion |
There was a problem hiding this comment.
This fielld should be non-nullable. I created separate PR about the database schema.
#9187
There was a problem hiding this comment.
Ok. Can you do rebase? My change has been merged.
There was a problem hiding this comment.
I hurriedly pushed now. I will still come back to it this night. e40d532
|
Do you want to add something? I would like to merge this change, but it is a draft, so I can't do it. |
ephraimbuddy
changed the title
|
You can merge it. I don't have anything to add again |
|
Thanks @ephraimbuddy ! |
4 participants
Closes: #8127
Make sure to mark the boxes below before creating PR: [x]
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.