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

The Big Refactor #166

Merged
merged 71 commits into from
Aug 22, 2023
Merged

The Big Refactor #166

merged 71 commits into from
Aug 22, 2023

Conversation

KeesCBakker
Copy link
Collaborator

@KeesCBakker KeesCBakker commented Aug 17, 2023

This big refactor is about the following:

  • Solving Refactor platform specific actions to adapters? #165, it introduces the concept of the Adapter. If a platform needs a specific Uploader or Responder, we can create an adapter for that. This helps in isolating the code that is needed for a certain platform and it helps us to test platform-specific code.
  • Partially solving [Idea] Split core Grafana interactions into its own node module #48, by introducing a GrafanaClient that is a small wrapper over the Hubot HTTP to aide with interactions with Grafana. The client is created in the createGrafanaClient function and it takes care of the HUBOT_GRAFANA_PER_ROOM setting based on the response.
  • Solving Tests are too slow or timing out #164, the old tests were not isolated enough, so there were some interactions. The new tests are based on the hubot-mock-adapter and a small wrapper (more here: https://keestalkstech.com/2023/08/hubot-testing-revisited/). We leveraged Promises / async / await instead of using setTimeout. All tests will now finish within a second, which makes committing and pushing better. By moving some of the testcases to the TestBot, we don't have to setup all the tests every time. By dropping hubot-test-helper, we're able to test against version 7 of Hubot (earlier versions had problems with the Log feature). We can now also drop sinon, require-dir and sinon-chai. The new test setup has no knowledge of rooms and users -- when I look to the testcases we don't need them.
  • Added typings in JSDocs to help to navigate those Hubot objects properly.
  • The SlackUploader now uses the Slack Web Client provided by the adapter to do the uploading. Basically the same idea the TelegramUploader does.
  • Solve Fetch entire dashboard instead of panels #72 by adding a kiosk parameter. Added the test cases as well.

With this refactor there comes some ifs and buts:

  • In the previous version I found some code that I think could not work (see Refactor platform specific actions to adapters? #165). That's why I refactored the setup a bit, we first download the file, before we try to upload it. The only way to get a response is to have an Uploader, which are S3, Slack, RocketChat or Telegram.
  • I could test S3 upload with Slack and Slack Upload with Slack, but I could not test the rest of the setup, as I don't have a BearyChat, HipChat, Telegram or RocketChat setup. I believe the tests do not capture the broken code I mentioned before. To be clear: the tests are green.
  • After introducing typings I saw that our msg object was not the message, but the res object, which contains a message. So I refactored some of those references. Not all of them, because that would muddy the PR a bit, we need to do that later.
  • Not sure why, but linting became a mess. When I resolve the lint statements, it will get rid of my typings, so I don't know what to do there...

This has been a huge refactor, but I think it helps us with going forward. I think we should test some more platforms before releasing. When we release let's make sure we release a major version, so everyone can double check before upgrading.


Fixes #165
Fixes #164

To make sure things are done right we use the Hubot types. msg was not really the message, but the response object, so we renamed it to 
es.
@stephenyeargin
Copy link
Owner

😲

Gonna set aside some time to review it with my full attention. If you don't hear from me by Monday, give me a nudge.

Removes the class and wraps everything in the one function scope. No `this` needed anymore.
Copy link
Owner

@stephenyeargin stephenyeargin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Ran it through a few tests with and without a registered adapter/uploader.

src/grafana.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/grafana.js Outdated Show resolved Hide resolved
@KeesCBakker
Copy link
Collaborator Author

Improved some tests, found some extra blind spots to fix

image

@KeesCBakker
Copy link
Collaborator Author

Also added kiosk mode for dashboards, so we fix #72

image

@KeesCBakker
Copy link
Collaborator Author

@stephenyeargin would you be able to test RocketChat, BearyChat and Telegram to see if they are still OK?

We're not testing the timezone here, so it should not be included.
@stephenyeargin
Copy link
Owner

stephenyeargin commented Aug 20, 2023

@stephenyeargin would you be able to test RocketChat, BearyChat and Telegram to see if they are still OK?

So, those were introduced by other contributors, I don't actually have accounts with any of them. Can tag folks and give them some time to give it a spin. Generally speaking, if the tests pass, I'm not too worried about a regression. 😁

BearyChat - @shonenada via #65
RocketChat - @mfilotto via #70
Telegram - @pinguingman via #135

@KeesCBakker
Copy link
Collaborator Author

I'm okay with merging and publishing.

@KeesCBakker KeesCBakker merged commit 7d360b2 into stephenyeargin:main Aug 22, 2023
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.

Refactor platform specific actions to adapters? Tests are too slow or timing out
2 participants