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

Gitlab Client: Parse owner and repository in a url safe way #698

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

StingRayZA
Copy link

Instead of stitching the 'owner' and 'repository' together with a %2F - rather join them with a / and let python make them conformant.
This way, if we have any / characters embedded in either value, they too will be correctly translated

@sduenas
Copy link
Member

sduenas commented Oct 23, 2020

Thanks for the PR. I need some time to review it. I'll do it this weekend.

@StingRayZA
Copy link
Author

Hi 👋
Could I request you label this PR with hacktoberfest-accepted ?

@sduenas
Copy link
Member

sduenas commented Oct 30, 2020

@StingRayZA I cannot merge the PR right now because it will break other parts of the tool chain. They expect the URLs as they are now. Would you mind to rebase your PR to master and improve the commit message. It is described in the contributing rules.

I won't merge it but I will set it as a hacktober contribution.

@StingRayZA
Copy link
Author

Sure, no problem I'll rebase and reformat the commit message.

This allows us to retain the '/' separation of URL pieces in any gitlab
urls that have been stored in the object. This change also lays the
groundwork for a fix in the -elk repository to allow multiple nested
gitlab group layers in gitlab URLs.

See also: chaoss/grimoirelab-elk#946

Signed-off-by: Raimund Hook <[email protected]>
@StingRayZA
Copy link
Author

@sduenas Rebased on master with a reformatted message.
It also links to chaoss/grimoirelab-elk#946 which is where I started thinking about this issue. I think my hack in that issue is a bit naive, but together with this one - my local copy of grimoirelab is working well now.

Copy link
Member

@sduenas sduenas left a comment

Choose a reason for hiding this comment

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

LGTM

As I said, I'll merge it when we fix the related problems in ELK.

@sduenas sduenas added the hacktoberfest-accepted Hacktoberfest tasks 2020 label Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Hacktoberfest tasks 2020
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants