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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the correct branch name in the badges #102

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

marksalpeter
Copy link

@marksalpeter marksalpeter commented Apr 21, 2021

馃憢 @posener. We're using this action to generate our readme over here and I noticed that all of our badges were broken (our master branch is called main).

I added a feature that renders the HEAD branch name in the template by default or a branch override flag. I also added a GitHub workflow bage, and a flag to override the default commit message. Let me know what you think.

Finally, the checks appear to be failing, but I don't understand why. All of the tests are passing locally.

@posener
Copy link
Owner

posener commented Apr 23, 2021

Hi Mark.
Thanks for using goreadme and submitting a fix for the issue.
It was not me that was responding, but the goreadme/goaction bots. I should fix this such that it will look like a bot and not like a person responding.

I see the issue you are suggesting on.
As for the solution:
I think that exec.Command should be used as a last resort.
In this case, I think you can use the goaction.Branch() value instead. See https://github.com/posener/goaction/blob/master/goaction.go#L237.
Can you update your change and see if it works for you?

@marksalpeter
Copy link
Author

Thanks @posener, I'll make the change tomorrow.

@marksalpeter
Copy link
Author

馃憢 Hello agian @posener!

I referenced this fork by commit SHA in our repo. You can now see all of the features this fork has added in the readme file of the @deliveryhero pipelines repo.

This version uses goaciton.Branch() first and then falls back to git commands for the compiled local version. Let me know what you think when you get the chance.

Thanks again for the feedback 馃檹

@posener
Copy link
Owner

posener commented May 21, 2021

Hi Mark,

I think this PR is currently mixing multiple things. The branch name issue, adding badges, allowing modify commit messages. Usually one would create a different independent PR for each of these.

Scoping back to the branch name issue. I think that the failure you get is because the call uses a the env variables from Github action environment. You should protect the call with if goaction.CI { ... }. You can read more at https://github.com/posener/goaction

@marksalpeter
Copy link
Author

Thanks again for the feedback @posener!

You're right, this isn't a very clean PR. I just implemented all of the badge features we needed in one go. It would have been better in hind-sight if I had made separate branches and PRs for the commit message override, the github actions badge, and the branch name fix.

It looks like the repo has changed significantly since my fork and there are merge conflicts now. We'll just keep using my fork for now and if you ever get around to incorporating these features into the action we'll switch back. Feel free to close this PR.

Best,
-Mark

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants