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
Construct pushURL from remote instead of hardcoding github.com #94
Conversation
Signed-off-by: Reed Palmer <reed.d.palmer@gmail.com>
closes #91 |
Signed-off-by: Reed Palmer <reed.d.palmer@gmail.com>
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 <reed.d.palmer@gmail.com>
Signed-off-by: Reed Palmer <reed.d.palmer@gmail.com>
@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.