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(gitlab): URL-encode the owner in remote requests for GitLab #742

Merged
merged 1 commit into from
Jul 28, 2024

Conversation

tonybutt
Copy link
Contributor

@tonybutt tonybutt commented Jul 3, 2024

Description

Gitlab requires that the project path be fully encoded. See the docs for details. https://docs.gitlab.com/ee/api/rest/#namespaced-path-encoding Pulls in the url encoding crate here. Might not be needed when you have the choice to just update the documentation. I don't think the end user should be required to url encode their own strings. It might be nicer to just add the subgroup paradigm to the gitlab config in general for the future. This will fix the problem for now.

Motivation and Context

I want to use git cliff on my self hosted gitlab.
#687

How Has This Been Tested?

I wrote a test. Doesn't cover the case on IF the end user url encodes their own string. This will break unexpectedly, but I rushed this in.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (no code change)
  • Refactor (refactoring production code)
  • Other

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.
  • I have formatted the code with rustfmt.
  • I checked the lints with clippy.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Gitlab requires that the project path be fully encoded. See the docs for
details. https://docs.gitlab.com/ee/api/rest/#namespaced-path-encoding
Pulls in the urlencoding crate here. Might not be needed when you have
the choice to just update the documentation. I don't think the end user
should be required to url encode their own strings. It might be nicer to
just add the subgroup paradigm to the gitlab config in general for the
future. This will fix the problem for now.
@tonybutt tonybutt requested a review from orhun as a code owner July 3, 2024 06:00
Copy link

welcome bot commented Jul 3, 2024

Thanks for opening this pull request! Please check out our contributing guidelines! ⛰️

@tonybutt
Copy link
Contributor Author

tonybutt commented Jul 3, 2024

I use nix so getting onto nightly rust fmt/clippy was non trivial for me tonight.

Feel free to do the clean up. There were lots of failing tests so I only ensured mine passed locally. I imagine I am missing some setup for those tests specifically, but maybe I broke them however my change seems unrelated. More importantly running git cliff with my changes produced the desired outcome in the issue.

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.78%. Comparing base (227a307) to head (a2905b4).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #742      +/-   ##
==========================================
+ Coverage   36.71%   36.78%   +0.07%     
==========================================
  Files          19       19              
  Lines        1501     1501              
==========================================
+ Hits          551      552       +1     
+ Misses        950      949       -1     
Flag Coverage Δ
unit-tests 36.78% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tonybutt
Copy link
Contributor Author

tonybutt commented Jul 4, 2024

There are some additional problems related here. Encoding subgroups is one and I'd like to keep this PR pure to that particular issue. I am going to open another PR for the deserialize problem.

Copy link
Owner

@orhun orhun left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@orhun orhun changed the title fix: Encode owner attr in remote for gitlab fix(gitlab): URL-encode the owner in remote requests for GitLab Jul 28, 2024
@orhun orhun merged commit e3e7c07 into orhun:main Jul 28, 2024
52 of 54 checks passed
Copy link

welcome bot commented Jul 28, 2024

Congrats on merging your first pull request! ⛰️

@orhun orhun mentioned this pull request Jul 28, 2024
1 task
@orhun orhun mentioned this pull request Sep 18, 2024
1 task
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.

3 participants