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

Fix 4120: Repository name cannot contain '/' character anymore #4279

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

LucasG0
Copy link
Contributor

@LucasG0 LucasG0 commented Sep 6, 2024

Fix #4120.

Note the current error message displayed to the user might not be very friendly. I am not sure whether we are fine with this or if we want to display a clearer one.
image

Also note that as discussed with @ogenstad the long term fix for this issue might be to store a repository using its id instead of name attribute so / characters could not be considered as file path separators.

This PR currently targets stable but we might want to re-target it to 0.16.1 branch once created.

@LucasG0 LucasG0 requested a review from a team September 6, 2024 16:15
@github-actions github-actions bot added the group/backend Issue related to the backend (API Server, Git Agent) label Sep 6, 2024
Copy link
Contributor

@ogenstad ogenstad left a comment

Choose a reason for hiding this comment

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

LGTM. While it would be good with a more human friendly error message I think this is something that relates to these regex constraints as a whole and it's not specifically tied to this PR. At least here the cure is better than the strange error we had before.

I'll also create an issue to track the transition to storing the repos on disk with "id" instead of "name", at that point we can remove this regex again.

Other than that I'm just not certain where we should merge this for now as it's so close to the 0.16.0 release or if this would go to the 0.16.1. I think either or is fine. We can sync that during the day. It would be nice to add a towncrier newsfragment stating that this fixes #4120.

@LucasG0 LucasG0 merged commit b923f65 into stable Sep 9, 2024
43 checks passed
@LucasG0 LucasG0 deleted the lgu-forbid-slash-in-repo-name branch September 9, 2024 09:38
@LucasG0
Copy link
Contributor Author

LucasG0 commented Sep 9, 2024

Merging as discussed with @dgarros.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
group/backend Issue related to the backend (API Server, Git Agent)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: repository fails sync when it has a / character in the name attribute
4 participants