Skip to content

Conversation

@vit-zikmund
Copy link
Collaborator

@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
Collaborator 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
Collaborator 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
Collaborator 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
@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