-
Notifications
You must be signed in to change notification settings - Fork 116
Construct pushURL from remote instead of hardcoding github.com #94
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
Conversation
Signed-off-by: Reed Palmer <[email protected]>
closes #91 |
Signed-off-by: Reed Palmer <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this slipped through.
return "", err | ||
} | ||
|
||
pushURLArray := strings.SplitAfter(strings.TrimSpace(string(pushURL)), "https://") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably won't work for SSH URLs. Please add a test that verifies the functionality for both, SSH and HTTPS URLs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right regarding SSH. That's not supported and a token is needed for pushing. chart-releaser is usually used with GitHub Actions which don't use SSH anyways.
Thanks for the review @unguiculus. I will pick this back up soon. FWIW, the original functionality did not work with SSH either, but regardless it would be good to support, and I should add tests. |
@rdpa i run into the same issue any news on that? |
Any updates on this one? Having the same issue here. |
Signed-off-by: Reed Palmer <[email protected]>
Signed-off-by: Reed Palmer <[email protected]>
Signed-off-by: Reed Palmer <[email protected]>
@unguiculus tests added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm this change, dont see big issue
No description provided.