-
Notifications
You must be signed in to change notification settings - Fork 964
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: working POST API for Project Observations #15228
feat: working POST API for Project Observations #15228
Conversation
ae9c8e8
to
45d016c
Compare
45d016c
to
14aec56
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really nice and straightforward PR thanks to a lot of previous legwork. Thanks for that @miketheman.
Overall I will be confident in approving this for merge once we have a couple of things inplace:
- Sufficiently annoying/obvious signs to users that we are not committing to any API ergonomics or features. Think "danger" and "unstable" and "preview" and "dragons here" type verbiage in versions and or urls.
- A confidence in our malware observation payloads such that we feel comfortable that they will be actionable immediately, and not too likely to be discarded or useless long term. I see that [DRAFT] PyPI Observation Reporting Payload #14503 has some good updates on approaches here. Thanks for that.
- A side-effect to committing a malware observation that creates a conversation in the PyPI Security inbox. This ensures that any reporters that adopt the API won't have their reports "lost". It will integrate this effort into our current pipeline fairly seamlessly! I recommend attaching the observation as a
.json
attachment given... - Some documentation that observations associated with deleted objects will also be deleted until we implement soft-deletes.
f5336f9
to
4551d1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review!
I've addressed the first couple of bullets so far, and will continue to work on the next two.
767420a
to
c4185e9
Compare
This is pretty much ready to go, while I work out some more details in a follow up, unless reviewers see them as merge blockers:
Keep in mind: Only superusers or folks we've set |
Migration to ship separately, before this code is active. Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Includes many TODOs, but works. Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Signed-off-by: Mike Fiedler <miketheman@gmail.com>
So that it's clearer these are dangerous. Signed-off-by: Mike Fiedler <miketheman@gmail.com>
c4185e9
to
a6f76fa
Compare
Signed-off-by: Mike Fiedler <miketheman@gmail.com>
a6f76fa
to
5a5acea
Compare
Signed-off-by: Mike Fiedler <miketheman@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I am very impressed/happy with the iteration on our API infrastructure/machinery here!
I have one concern regarding local development that I think would be wise to consider before merge just so that the feature can be worked on by anyone, not just those of us with HS access.
A simple bail-out clause that prints instead of makes HTTP calls if the user hasn't provided credentials. Signed-off-by: Mike Fiedler <miketheman@gmail.com>
Please review commit-by-commit to make reviewing easier.
Notes:
GET /danger-api/echo
endpointPOST /danger-api/projects/{name}/observations
endpoint that validates the inbound payload (kinda sorta) and persists in the databaseis_observer
will be allowed to POST. This is not a good long-term solution, but can likely work well enough to gather some data.Does NOT include any tests yet - I wanted to get some structural approaches done here first.Tests! But they rely on the migration PR being merged first.