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

Fix for slackdiff. #3144

Merged
merged 4 commits into from
Jun 17, 2024
Merged

Fix for slackdiff. #3144

merged 4 commits into from
Jun 17, 2024

Conversation

Punicaa
Copy link
Contributor

@Punicaa Punicaa commented Apr 29, 2024

Pre-Request Checklist

  • Passes rubocop code analysis (try rubocop --auto-correct)
  • Tests added or adapted (try rake test)
  • Changes are reflected in the documentation
  • User-visible changes appended to CHANGELOG.md

Description

  • Updated slackdiff.rb to use slack_ruby_client instead of slack-api.
  • Updated default timeout from 20 to 60.

Fixes users experiencing the following error:

ERROR -- : Hook slack (#SlackDiff:0x00005572c9621e20) failed (#<NameError: uninitialized constant Slack::Client>) for event :post_store #2697

@robertcheramy
Copy link
Collaborator

I miss the update of the default timeout in the diff. Can you check that?
Can you also merge master into your branch (or rebase it, as you which), and modify Changelog so that the changes are under the unreleased Version?

@Punicaa
Copy link
Contributor Author

Punicaa commented Jun 11, 2024

Thanks for the comment. Changes from Master pulled and made changes to Changelog and actually changed the timeout.

@robertcheramy
Copy link
Collaborator

Something whent terribly wrong here, as many commits where integrated in this PR. Mabe a git rebase would bring some clarity in the PR?

@robertcheramy
Copy link
Collaborator

Why should the default timeout be changed? If you don't like it, you can change it in your configuration.

@robertcheramy
Copy link
Collaborator

Something went terribly wrong here, as many commits where integrated in this PR. Mabe a git rebase would bring some clarity in the PR?

I've rebased your branch. Be sure to update your branch before submitting more commits. You may need a `git reset --hard.

@Punicaa
Copy link
Contributor Author

Punicaa commented Jun 14, 2024

Thanks for fixing the branch. The timeout of 20 seconds is too few for devices with more complicated configurations, like firewalls, or some switches. It's also on the low-end when devices need to be pulled over higher latency. When setting up oxidized in our environment it was "broken" out of the box because of this. I figured this could help some first time users, but I can also leave it out and we'll just go with the slack fix.

@robertcheramy
Copy link
Collaborator

You can change the timeout in your configuration file:

timeout: 20

I'll revert the timeout change an merge into master.

@robertcheramy robertcheramy merged commit f37ceea into ytti:master Jun 17, 2024
5 checks passed
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