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.

Sign up for GitHub

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

Jump to bottom

added initial apikey endpoint #26

Closed
wants to merge 9 commits into from
Closed

added initial apikey endpoint #26

wants to merge 9 commits into from

Conversation

brngylni
Copy link
Contributor

@brngylni brngylni commented Jul 2, 2024

This PR adds the apikey endpoint #6

@gridhead gridhead self-assigned this Jul 3, 2024
@gridhead gridhead requested review from abompard and gridhead July 3, 2024 07:06
gridhead
gridhead requested changes Jul 8, 2024
View reviewed changes
Copy link
Member

@gridhead gridhead left a comment

Choose a reason for hiding this comment

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

Please have the __init__.py and util.py modules of the endpoints directory in a separate pull request. A bunch of commits are redone here that are already done before with the inclusion of these files.

Also, please follow #2 (comment).

webhook_to_fedora_messaging/endpoints/apikey.py
Comment on lines +2 to +4
from ..database import db
from ..models.apikey import APIKey
from ..models.user import User
Copy link
Member

Choose a reason for hiding this comment

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

Ref. #24 (comment)

webhook_to_fedora_messaging/endpoints/apikey.py Show resolved Hide resolved
webhook_to_fedora_messaging/endpoints/apikey.py Outdated Show resolved Hide resolved
webhook_to_fedora_messaging/endpoints/apikey.py Outdated Show resolved Hide resolved
webhook_to_fedora_messaging/endpoints/apikey.py Show resolved Hide resolved
webhook_to_fedora_messaging/endpoints/util.py Outdated Show resolved Hide resolved
webhook_to_fedora_messaging/endpoints/util.py Outdated Show resolved Hide resolved
webhook_to_fedora_messaging/endpoints/util.py Outdated Show resolved Hide resolved
webhook_to_fedora_messaging/endpoints/util.py Outdated Show resolved Hide resolved
webhook_to_fedora_messaging/endpoints/util.py Outdated Show resolved Hide resolved
@brngylni brngylni requested a review from gridhead July 9, 2024 22:53
gridhead
gridhead requested changes Jul 10, 2024
View reviewed changes
Copy link
Member

@gridhead gridhead left a comment

Choose a reason for hiding this comment

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

Once you are done with addressing the newly suggested changes, please squash the commits into just one. Every other change can be added after this is merged.

webhook_to_fedora_messaging/endpoints/apikey.py Outdated
Comment on lines 83 to 95




Copy link
Member

Choose a reason for hiding this comment

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

Please do not mark comments as resolved - especially when the suggested change is not addressed. Instead of leaving five lines at the end of the line - please leave just one line.

@brngylni
Copy link
Contributor Author

Once you are done with addressing the newly suggested changes, please squash the commits into just one. Every other change can be added after this is merged.

should I squash all commits so far into one or should I squash them into 1 after every review?

@gridhead
Copy link
Member

@brngylni Once you get an LGTM from either my end or from @abompard, you can proceed with the squash.

@brngylni brngylni requested a review from gridhead July 11, 2024 13:16
@brngylni
Copy link
Contributor Author

Please have the __init__.py and util.py modules of the endpoints directory in a separate pull request. A bunch of commits are redone here that are already done before with the inclusion of these files.

Also, please follow #2 (comment).

#32

@brngylni brngylni force-pushed the dev_apikey_endpoint branch from 95c2f63 to 6b79a7a Compare July 12, 2024 10:19
brngylni and others added 2 commits July 12, 2024 14:31
@brngylni
added initial apikey endpoint
76d852a
@gridhead @brngylni
Add SQLAlchemy Helpers integration
2c0e3db 
Signed-off-by: Akashdeep Dhar <[email protected]>

Fix the Vagrant environment

Signed-off-by: Aurélien Bompard <[email protected]>

resolve review

delete __init__ and util
@brngylni brngylni force-pushed the dev_apikey_endpoint branch from 6b79a7a to 2c0e3db Compare July 12, 2024 11:31
gridhead
gridhead requested changes Jul 15, 2024
View reviewed changes
Copy link
Member

@gridhead gridhead left a comment

Choose a reason for hiding this comment

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

Please update the branch to ensure that you keep up with the rebasing merge and once that is done, only include the codebase relevant to this pull request here. Finally, squash the commits and let me know in the channel so I can review it again.

@brngylni brngylni requested a review from gridhead July 15, 2024 13:06
@brngylni
Copy link
Contributor Author

Please update the branch to ensure that you keep up with the rebasing merge and once that is done, only include the codebase relevant to this pull request here. Finally, squash the commits and let me know in the channel so I can review it again.

How do I fix the interfering commits like in this branch?

@brngylni brngylni closed this Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abompard abompard Awaiting requested review from abompard

@gridhead gridhead Awaiting requested review from gridhead

Assignees

@gridhead gridhead

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@brngylni @gridhead