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

Change code to use subgroup in gitlab #80

Merged
merged 1 commit into from Nov 17, 2020
Merged

Conversation

Joda89
Copy link

@Joda89 Joda89 commented Nov 16, 2020

When I use the subgroup of gitlab I get an error "Incorrectly formatted git address"
the id on gitlab can use more than two block
I changed the code to be able to handle two or more blocks in the ID
Can you tell me if this is right for you?

@Joda89 Joda89 marked this pull request as ready for review November 16, 2020 19:56
@stefanprodan
Copy link
Member

@Joda89 please run make test and commit changes, then squash your commits into a single one. Thanks!

@Joda89 Joda89 force-pushed the patch-1 branch 3 times, most recently from bc2256d to 8e4cf16 Compare November 17, 2020 08:56
@Joda89
Copy link
Author

Joda89 commented Nov 17, 2020

@stefanprodan It's done

@phillebaba
Copy link
Member

@Joda89 could you also add a test in util_test.go for a GitLab url with a subgroup. You should be able to just copy one of the existing tests.

@Joda89
Copy link
Author

Joda89 commented Nov 17, 2020

@phillebaba it's done

@phillebaba
Copy link
Member

Sorry for not thinking about subgroups in GitLab, there are always small detail differences between the different GitHub providers. Thank you for taking the time and fixing it. 🌹

Now when I think about it I don't really remember why I thought it was a good idea to split the path into components.
Could you change the code to just clean up the id value and return it?

func parseGitAddress(s string) (string, string, error) {
	u, err := giturls.Parse(s)
	if err != nil {
		return "", "", nil
	}

	id := strings.TrimLeft(u.Path, "/")
	id = strings.TrimSuffix(id, ".git")
	host := fmt.Sprintf("https://%s", u.Host)
	return host, id, nil
}

Each provider should be responsible for verifying the repo id they get so that it matches their client. GitHub needs to split it while GitLab does not. I can add that validation tonight to the different providers if you make my suggested changes so we merge this PR.

Signed-off-by: Johan Lore <jlore@ippon.fr>
@Joda89
Copy link
Author

Joda89 commented Nov 17, 2020

@phillebaba It's push
tell me if it's good for you ?

Copy link
Member

@phillebaba phillebaba left a comment

Choose a reason for hiding this comment

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

LGTM 🥇

I will add some tests and then hopefully we can release notification-controller soon so the GitLab notifier works for you.

@Joda89
Copy link
Author

Joda89 commented Nov 17, 2020

thanks

@phillebaba phillebaba merged commit 97184e8 into fluxcd:main Nov 17, 2020
@Joda89 Joda89 deleted the patch-1 branch November 17, 2020 12:24
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.

None yet

3 participants