Skip to content

fix: Correctly retrieve GitHub enterprise url#858

Merged
olblak merged 14 commits into
updatecli:mainfrom
olblak:issue/854
Sep 10, 2022
Merged

fix: Correctly retrieve GitHub enterprise url#858
olblak merged 14 commits into
updatecli:mainfrom
olblak:issue/854

Conversation

@olblak

@olblak olblak commented Sep 9, 2022

Copy link
Copy Markdown
Member

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

@olblak
fix: correctly retrieve github enterprise url
11e7f61 
Signed-off-by: Olblak <me@olblak.com>
@olblak olblak requested a review from dduportal September 9, 2022 07:26
@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

gerardsegarra commented Sep 9, 2022

Copy link
Copy Markdown
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

@olblak
Remove hardcode github.com from changelog
5fe9c23 
Signed-off-by: Olblak <me@olblak.com>
@olblak
Allow to both http and https schema for Github URL
e8b04c3 
Signed-off-by: Olblak <me@olblak.com>
@olblak

olblak commented Sep 9, 2022

Copy link
Copy Markdown
Member Author

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

@olblak
fix: url scheme
9d7c7a7 
Signed-off-by: Olblak <me@olblak.com>
@olblak
remove .git from changelog debug message
68e3193 
Signed-off-by: Olblak <me@olblak.com>
@olblak
Add Github scm test cases
96637dc 
Signed-off-by: Olblak <me@olblak.com>
@olblak

olblak commented Sep 9, 2022

Copy link
Copy Markdown
Member Author

@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

@olblak
fix: githubrelease tests
5bd6aa0 
Signed-off-by: Olblak <me@olblak.com>
@olblak olblak mentioned this pull request Sep 9, 2022
Closed
lemeurherve
lemeurherve previously approved these changes Sep 10, 2022
View reviewed changes

@lemeurherve lemeurherve left a comment

Copy link
Copy Markdown
Member

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

Comment thread pkg/plugins/scms/github/changelog.go Outdated
Comment thread pkg/plugins/scms/github/changelog_test.go Outdated
@olblak

olblak commented Sep 10, 2022

Copy link
Copy Markdown
Member Author

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

gerardsegarra commented Sep 12, 2022

Copy link
Copy Markdown
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

Reviewers

@dduportal dduportal Awaiting requested review from dduportal

1 more reviewer

@lemeurherve lemeurherve lemeurherve approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Cannot get repo from enterprise github

3 participants

@olblak @gerardsegarra @lemeurherve