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: Correctly retrieve GitHub enterprise url #858

Merged
merged 14 commits into from
Sep 10, 2022
Merged

Conversation

olblak
Copy link
Member

@olblak olblak commented Sep 9, 2022

Signed-off-by: Olblak me@olblak.com

Fix #854

Test

To test this pull request, you can run the following commands:

# Build binary using Golang 1.19
go build -o bin/updatecli .
# Test using a manifest
./bin/updatecli diff --config updatecli/updatecli.d

Additional Information

Tradeoff

I don't have access to a GitHub enterprise service to test. But we probably just read to quickly this doc https://github.com/shurcooL/githubv4#usage

Potential improvement

Signed-off-by: Olblak <me@olblak.com>
@olblak olblak changed the title fix: Correctly retrieve github enterprise url fix: Correctly retrieve GitHub enterprise url Sep 9, 2022
@olblak olblak added the bug Something isn't working label Sep 9, 2022
@gerardsegarra
Copy link
Contributor

Oh, I see you've already been working on it. I just created this PR #859, but I'm not an expert either in golang or updatecli code :P

Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
@olblak
Copy link
Member Author

olblak commented Sep 9, 2022

Oh, I see you've already been working on it. I just created this PR #859, but I'm not an expert either in golang or updatecli code :P

Your PR is better as it has tests, so I'll do some suggestion to yours :p

Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
Signed-off-by: Olblak <me@olblak.com>
@olblak
Copy link
Member Author

olblak commented Sep 9, 2022

@gerardsegarra In the end I think my PR is in a more advanced state even thought we had the same thing in mind.

I improved the spec so we can also specify http protocol in the URL. If no protocol is specified then it fallbacks to https

Additional work would be needed if we want to allow the ssh:// protocol

Signed-off-by: Olblak <me@olblak.com>
lemeurherve
lemeurherve previously approved these changes Sep 10, 2022
Copy link
Member

@lemeurherve lemeurherve left a comment

Choose a reason for hiding this comment

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

GitHub correct case (nit)
LGTM otherwise

pkg/plugins/scms/github/changelog.go Outdated Show resolved Hide resolved
pkg/plugins/scms/github/changelog_test.go Outdated Show resolved Hide resolved
@olblak
Copy link
Member Author

olblak commented Sep 10, 2022

Thanks @lemeurherve for the review and thanks @gerardsegarra for spotting this issues

@olblak olblak merged commit 745757c into updatecli:main Sep 10, 2022
@olblak olblak deleted the issue/854 branch September 10, 2022 11:07
@gerardsegarra
Copy link
Contributor

Thank you so much for fixing it! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot get repo from enterprise github
3 participants