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: use endpoints compatible with LFS server discovery #152

Merged
merged 4 commits into from
Mar 11, 2024
Merged

feat: use endpoints compatible with LFS server discovery #152

merged 4 commits into from
Mar 11, 2024

Conversation

vit-zikmund
Copy link
Contributor

@vit-zikmund vit-zikmund commented Mar 6, 2024

Introducing the base URI via the BaseView seems like a nice and consistent way of controlling this.

This change additionally adds support for the <organization> placeholder in the URI to contain slashes, as some git repositories (e.g. those GitLab) contain multiple levels of organization groups/namespaces.

As it currently stands in the PR, the whole such string ends up in the organization variable, which I feel, became a bit misleading. I'd recommend refactoring the code to use something like org_path, but wanted to hear your opinions.

Closes: #151

@athornton
Copy link
Collaborator

Unless I am misunderstanding, this would require everyone using giftless to reconfigure their git LFS urls in all their LFS-enabled repositories.

Am I misunderstanding?

If I am not, this needs to be behind a feature flag which is disabled by default. It would be nice to bring giftless into line with what it's supposed to be doing, but not at the cost of breaking absolutely everyone who currently uses it and didn't pay close attention to the release notes.

Doing it as a feature flag with a note that the new behavior will become the default at some point in the future, and the current endpoints will be deprecated would be OK.

@vit-zikmund
Copy link
Contributor Author

OK, that's more than fair. Feature flag incoming.

Copy link
Collaborator

@athornton athornton left a comment

Choose a reason for hiding this comment

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

This looks awesome.

@athornton
Copy link
Collaborator

If no one from Datopian objects I'll merge it tomorrow.

@vit-zikmund
Copy link
Contributor Author

Please don't merge yet, just discovered Werkzeug kinda fails at parsing the /<repo>.git/ part, where the repo variable actually gets the full string between slashes. I'll have to work around it. But it's too early today for me to do anything more on this, gotta chase the missed yesterday's sleep.

@athornton
Copy link
Collaborator

Cool. You will want to rebase, as I merged a couple of smaller PRs (one of them yours) before getting here.

Additionally, add support for the <organization> in the URI to contain slashes, as some git repositories(e.g. GitLab) can contain multiple levels of organization groups/namespaces.
This entails a dynamically created class voodoo for the change not to poison too much of the existing code.

We should add test alternatives for those currently changed that they work properly with the LEGACY_ENDPOINTS turned off.
@vit-zikmund
Copy link
Contributor Author

vit-zikmund commented Mar 8, 2024

I should be done here. I've added the handful of promised tests for the legacy endpoints and discovered, that the previous error I was seeing (and blamed to Werkzeug) is actually my bug I already fixed on the other PR and got confused on this branch, which was again forked from main :)

@athornton athornton merged commit 5838665 into datopian:main Mar 11, 2024
7 checks passed
@vit-zikmund vit-zikmund deleted the endpoints branch March 11, 2024 17:03
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.

[enhancement] Use automatic LFS server discovery endpoints
2 participants