-
Notifications
You must be signed in to change notification settings - Fork 113
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
Add option to push to gh-pages #73
Conversation
Signed-off-by: Reinhard Nägele <unguiculus@gmail.com>
I'd like to do a little refactoring in a follow-up PR in order to improve testability. With the current code, it is hard to add tests. |
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.
Some comments but overall this is great to add to the main tool. Looking forward to cleaning up helm/chart-releaser-action after this 👏
@@ -16,6 +16,7 @@ package cmd | |||
|
|||
import ( | |||
"github.com/helm/chart-releaser/pkg/config" | |||
"github.com/helm/chart-releaser/pkg/git" |
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.
@unguiculus what do you think about using https://github.com/go-git/go-git instead of our own package that shells out?
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.
We had used that in a client project and dropped it. We hit bugs and had various issues with it and it covers only a small subset of Git which wasn't enough for us. Plus, it's not really straight forward to use. I'd rather not use it.
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.
'worktree' is not supported BTW.
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.
I saw that in the compatibility doc, but read through https://github.com/go-git/go-git/blob/master/worktree_test.go, which led me to think the doc just wasn't updated. But I don't have any real world experience using that package, so definitely trust your judgement 👍 My main thought was it could be cool to be able to run this without git as a dependency. Maybe something to reevaluate later?
cr/cmd/index.go
Outdated
flags.String("pages-branch", "gh-pages", "The GitHub pages branch") | ||
flags.Bool("push", false, "Push index.yaml to the GitHub Pages branch (must not be set if --pr is set)") | ||
flags.Bool("pr", false, "Create a pull request for index.yaml against the GitHub Pages branch (must not be set if --push is set)") |
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.
Nice 👍
Cross-referencing discussion in helm/chart-releaser-action#33 about options design
pkg/releaser/releaser.go
Outdated
return true, nil | ||
} | ||
|
||
worktree, err := r.git.AddWorktree("", "origin/"+r.config.PagesBranch) |
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.
Don't want to scope creep too hard here, but we may now want to add a remote
config option as well?
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.
Good point. Added it.
Signed-off-by: Reinhard Nägele <unguiculus@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.
lgtm 🙌
This adds the option to push the
index.yaml
file, either directly to the GitHub Pages branch, which is now configurable, or via pull request.Signed-off-by: Reinhard Nägele unguiculus@gmail.com