-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Automate workflow to publish to RubyGems. #2584
Conversation
Signed-off-by: sudeeptarlekar <[email protected]>
.github/workflows/publish.yml
Outdated
push: | ||
tags: | ||
- "v*" | ||
pull_request: |
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 don't think we need to push gem to rubygems.org when we merge any PR to main branch, I added this for my testing purpose. Make sure we need this before merging.
Other things I think are quite self explanatory, the only part I was stuck/unsure of how are we going to handle secrete tokens?
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.
Makes sense. I'll remove the pull_request part.
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.
@vbrazo do we have the Ruby gem host key in the Settings? If not, we might need to add it for this to work.
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've just added 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.
hi @bijoysijo thank you for your contribution to Faker! I left a comment, let me know what you think.
I am a bit confused by the description of this PR. The branch was renamed to main a couple of days ago. Could you please update the description with the reason why behind the changes? Thanks!
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 is a nice addition, thanks for working on this!
We don't need the deployment-action parts though, so I left a few suggestions.
.github/workflows/publish.yml
Outdated
- uses: chrnorm/deployment-action@releases/v1 | ||
name: Create GitHub deployment | ||
id: deployment | ||
with: | ||
token: "${{ github.token }}" | ||
target_url: ${{ env.TARGET_URL }} | ||
environment: production |
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 don't have deployments or environments, so I don't think we need this step.
.github/workflows/publish.yml
Outdated
- name: Update deployment status (success) | ||
if: success() | ||
uses: chrnorm/deployment-status@releases/v1 | ||
with: | ||
token: "${{ github.token }}" | ||
target_url: ${{ env.TARGET_URL }} | ||
state: "success" | ||
deployment_id: ${{ steps.deployment.outputs.deployment_id }} | ||
|
||
- name: Update deployment status (failure) | ||
if: failure() | ||
uses: chrnorm/deployment-status@releases/v1 | ||
with: | ||
token: "${{ github.token }}" | ||
target_url: ${{ env.TARGET_URL }} | ||
state: "failure" | ||
deployment_id: ${{ steps.deployment.outputs.deployment_id }} |
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.
same thing here, we don't have deployments nor notifications, so this is not needed.
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'll update and push.
Hi, I've made the requested changes. Let me know if it needs more work. 😄 |
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, thanks!
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.
actually, forgot about one last thing: let's also add explicit permissions (see #2551 for reference)
@thdaraujo Everything look good? 😄 |
LGTM :) |
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.
🚀🚀🚀
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.
Thank you!
Just waiting on @vbrazo about the keys. |
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.
The RubyGem key was added.
LGTM 👍
Add Github action to automate publish to RubyGems on release / tag.
Fixes #2554