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

feat: add PAT support with custom Git server compatibility #69

Closed
wants to merge 3 commits into from

Conversation

cevatkerim
Copy link

@cevatkerim cevatkerim commented Dec 29, 2024

  • Add optional PAT field with toggle checkbox in UI
  • Support custom Git server API endpoints (api/v1/repos)
  • Add comprehensive test coverage for PAT authentication
  • Update repository existence check to handle both GitHub and custom Git servers

image

- Add optional PAT field with toggle checkbox in UI\n- Support custom Git server API endpoints (api/v1/repos)\n- Add comprehensive test coverage for PAT authentication\n- Update repository existence check to handle both GitHub and custom Git servers
Copy link
Owner

@cyclotruc cyclotruc left a comment

Choose a reason for hiding this comment

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

Very clean minimal implementaiton, I like that the tokens never leave RAM as it reduces the security risk of handling user tokens
Thank you very much

Copy link
Owner

@cyclotruc cyclotruc left a comment

Choose a reason for hiding this comment

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

My bad: I missed those 4 lines while merging and I can't seem to be able to push those changes on your PR

src/gitingest/tests/test_clone.py Outdated Show resolved Hide resolved
src/gitingest/tests/test_clone.py Outdated Show resolved Hide resolved
src/gitingest/tests/test_clone.py Outdated Show resolved Hide resolved
src/gitingest/tests/test_clone.py Outdated Show resolved Hide resolved
@cyclotruc
Copy link
Owner

@cevatkerim I was quite reluctant at first to introduce PAT in gitingest but this first implementation seems very reasonable
Thank you very much

I would just like to improve the UX a bit before merging, if you can give me access to your branch I will glaadly implement those changes myself:
image

  • The ingest button should remain as it was before on the same line as the search bar in desktop mode
  • Make the checkbox and new textinput look more like the include/exclude input (mostly smaller)

@cyclotruc
Copy link
Owner

Update:

Since gitingest is focused on butter smooth experience rather than having many features, I'm considering not pushing this first step (PAT) in production but instead wait until we have proper OAuth going on with Github login

I'm going to keep this branch as a reference implementation for PAT which I had not much experience with, so your work is still very helpful!

@cyclotruc cyclotruc closed this Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work in progress This PR is not ready yet but is being worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants