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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport #20 to version 1.x of the GEM #23

Merged
merged 4 commits into from
Jun 21, 2021
Merged

Backport #20 to version 1.x of the GEM #23

merged 4 commits into from
Jun 21, 2021

Conversation

pinkahd
Copy link

@pinkahd pinkahd commented Jun 21, 2021

Hey 👋🏼

We've encountered some issues when we've updated our sidekiq-unique-jobs gem to the latest version and it looks like the latest version started sending nested fields issue related #3 Currently we're still stuck on version 5 of Sidekiq so we couldn't update the sidekiq-logstash gem to version 2.x to get this patch in.

This patch is back porting the fix made in #20 to version 1.x as well, I'm thinking there might be other in the same situations that might benefit form this patch as well.

Let me know if you're still interested to support version 1.x of this gem if not I can close this PR and maintain our fork for now.

Thanks

@iMacTia iMacTia changed the base branch from master to 1.x June 21, 2021 15:46
@iMacTia
Copy link
Owner

iMacTia commented Jun 21, 2021

Hi @pinkahd, thanks for opening this.
I have nothing against back porting this fix and releasing a 1.2.1 version.
To do this, I've created a 1.x branch and set that as the base branch, however GitHub is not running the CI tests for some reason. Could you try pushing another commit to the PR, e.g. setting the ruby-version to 2.6.6?
Hopefully that will be enough to trigger the CI run

@pinkahd
Copy link
Author

pinkahd commented Jun 21, 2021

Pushed a commit to increase ruby version to 2.6.6

I believe the reason why CI isn't running it's because I'm a first time contributor.

Screenshot 2021-06-21 at 16 54 54

@iMacTia
Copy link
Owner

iMacTia commented Jun 21, 2021

I believe the reason why CI isn't running it's because I'm a first time contributor.

Correct, but GitHub was not showing me the "Approve and run CI" button, so I couldn't approve.
It did after your latest commit, as I imagined 😉
CI is running now 🙌 !

@iMacTia
Copy link
Owner

iMacTia commented Jun 21, 2021

@pinkahd minor rubocop offenses to fix, should be easy but please let me know if you need help 👍

@pinkahd
Copy link
Author

pinkahd commented Jun 21, 2021

Thanks 🙌🏼 I'll fix those and update the branch.

(cherry picked from commit ce5e099)
@pinkahd
Copy link
Author

pinkahd commented Jun 21, 2021

@iMacTia Looks like you'll need to approve the CI again after I've force update the branch, hopefully this this everything will pass 🤞🏼

@iMacTia
Copy link
Owner

iMacTia commented Jun 21, 2021

Mmmh something is wrong with bundle apparently, I noticed this line in the logs.

@pinkahd
Copy link
Author

pinkahd commented Jun 21, 2021

Mmmh something is wrong with bundle apparently, I noticed this line in the logs.

Hmm noticed that as well, could it be because of the depreciated actions/setup-ruby@v1 and version 2.5.x didn't had bundler installed 🤔

@pinkahd
Copy link
Author

pinkahd commented Jun 21, 2021

I think that was it, all checks passed with the new setup-ruby action 🎉

@iMacTia
Copy link
Owner

iMacTia commented Jun 21, 2021

Thanks for fixing that @pinkahd, all green now 🎉 ! LGTM

@iMacTia iMacTia merged commit 2942078 into iMacTia:1.x Jun 21, 2021
@pinkahd
Copy link
Author

pinkahd commented Jun 21, 2021

Thanks 🙌🏼 Let me know if you're gonna release version 1.2.1 in ruby gems 😇. Thanks again 💯

@iMacTia
Copy link
Owner

iMacTia commented Jun 21, 2021

https://rubygems.org/gems/sidekiq-logstash/versions/1.2.1 🎉

@pinkahd
Copy link
Author

pinkahd commented Jun 21, 2021

Amazing! 🎉

@pinkahd pinkahd deleted the 1.x branch June 21, 2021 16:37
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.

2 participants