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

[WIP] Rebuild and Improve Docker Container #289

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

Conversation

JSzaszvari
Copy link

@JSzaszvari JSzaszvari commented Jun 5, 2018

This is a WIP, Few minor changes needed before its good to go.

This is a complete refactor of the current Rocket.Chat Hubot Docker Container with a bunch of improvements.

Fixes #198 #236 #258 #262 #279 #237

GOOD TO KNOW:

  • Using HubotV3 and the new Rocket.Chat adapter, courtesy of @timkinnane
  • Container size has been reduced from 722mb to 139mb with no loss in features/functionality
  • Basic checks and output during container startup so that if someone forgets to set an environment var they will know what particular setting has not been set.
  • Container now handled SIGINT's properly and exits when it should

COMPLETE

TODO/IN PROGRESS:

  • Add a GIT_SCRIPTS env var so people can pull scripts into their container on startup from their own private git repos
  • Add a message to the current container's startup process so that it lets people know that there is a new version using hubotv3 and lots of other improvements. (don't want to break everyone using the hubot-rocketchat:latest image)
  • Update Documentation

Will update this list above over the next day or so, Making some changes and compiling a list of changes.

DISCUSS:

  • I think this should go into its own seperate git repo ( rocketchat-hubot3-docker or something like that) so that it's not in the same repo as the adapter? Thoughts? We will keep it in this repo and make it the default in order not to complicate things with too many unnecessary repos

  • Initially push this up to Docker hub with a v3 tag so rocketchat-hubot:v3 and not the default tag so we can get feedback and then once backward compatibility has been confirmed, we can push it to the default tag

@JSzaszvari
Copy link
Author

JSzaszvari commented Jun 5, 2018

Some screenshots of various things

The output on startup if something needs to be set that isn't

image

Info/Warnings on startup letting you know what isnt set so you know what your missing out on

image

@JSzaszvari
Copy link
Author

@timkinnane more to come over the next day, then everything should work well.

@geekgonecrazy @timkinnane / Anyone else - Any input/suggestions is welcome. Want to make sure this container is Uber Awesome

@JSzaszvari
Copy link
Author

Fancy banner added as well.. Because whats life without a bit of spice

image

@JSzaszvari
Copy link
Author

JSzaszvari commented Jun 6, 2018

@timkinnane I re-wrote the hubot-reload-scripts package to be in JS instead of CoffeeSCrap as i wanted it during testing. Turns out the NPM package "hubot-reload" dissapeared and only "hubot-reload-scripts" is online any more (hubot-reload is mine now! haha)

https://github.com/JSzaszvari/hubot-reload

I also combined the reload script + your test/diagnostic scripts into a hubot-rocketchat-diagnostics package and published to NPM

https://github.com/JSzaszvari/hubot-rocketchat-diagnostics

Did this to make my testing easier, dont need to use it, rather just saved me from having to gather them all repeatedly/list multiple packages

My current line of thinking is I want an environment variable for this docker image

ROCKETCHAT_BOT_DIAGNOSTICS=true

which then enables this package and a whole bunch of test tools without needing to source individual packages , your scripts, hubot-reload + a few others I'm yet to commit

@timkinnane
Copy link
Contributor

This is awesome stuff @JSzaszvari - thanks so much. Love the detail provided and the new banner most of all. I'm on leave for a couple weeks but I've checked this out and will test on my way somewhere.

@geekgonecrazy
Copy link
Member

This PR is fantastic! Massive amount of work! I'll try to take an in depth view of the docker image tomorrow 😁

@JSzaszvari
Copy link
Author

@geekgonecrazy @timkinnane

Thanks! Appreciate it.

Probably hold off testing till tomorrow as I've still got the last of the changes to merge. Ran out of time before work this morning, but will get the last of them up tonight.

Probably 90% through the rest of the outstanding items just ran out of time

@JSzaszvari
Copy link
Author

Copy link
Member

@geekgonecrazy geekgonecrazy left a comment

Choose a reason for hiding this comment

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

Maybe we can drop Dockerfile to root to dedup package.json?

Then just include the bashscript from sub folder?

@@ -0,0 +1,54 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Can we dedup this some how? Having both places is going to make updating version and other dependencies harder

Copy link
Author

Choose a reason for hiding this comment

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

Agreed that's where I had it last night, but this goes into my question of having a separate repo.

Do we want to merge the adapter and docker image like it is now in this, I think it should be separate as the adapter is really a separate component

Copy link
Author

Choose a reason for hiding this comment

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

Forgot to add, Have updated this with those changes. Life is easier now ;)

fi

echo "INFO: Attempting to install this containers dependancies"
npm install --no-audit
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this since you do during image build?

Copy link
Author

Choose a reason for hiding this comment

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

2 reasons

This is to combat a really strange issue I was seeing where during build it was not installing 1 or 2 random dependencies . Tried with yarn and npm, no joy. The extra 5 seconds this takes to just make sure is worth it in my eyes

And

When people specify new scripts with the EXTERNAL_SCRIPTS env var those deps are not installed at build and will need to be installed. As most won't be building it merely deploying from docker hub

Copy link
Author

Choose a reason for hiding this comment

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

Actually, the EXTERNAL_SCRIPTS is done separately

Ignore that second reason

Mainly the first, happy to try again without it but was seeing really strange dep issues and added it in as a just in case

@jangmarker
Copy link
Contributor

What is the progress on this? From my understanding this is still required and it looks pretty much done, doesn't it?

@JSzaszvari
Copy link
Author

@jangmarker Apologies I will have this completed sometime within the next week. I had started but then had a family matter that I needed to attend too overseas.

I am back now and will finish it up next week.

@Thibugs
Copy link

Thibugs commented Nov 20, 2018

Any update on the progress and an estimated date ? I'm actually stuck as we need to get the bot running on docker but the last release is too old.

@timkinnane
Copy link
Contributor

@tgalloy This PR is functional and perfectly fine to merge as far as I know, but no one is currently maintaining this adapter. I would merge it myself but I don't have time to test it and be on hand if it causes problems from any current users.

If you're stuck, you could fork and continue from John's version https://github.com/JSzaszvari/hubot-rocketchat - Then if you find and resolve any issues, you can contribute them back here, or at least comment to give validation on this PR.

@eloo
Copy link

eloo commented Jan 28, 2019

@timkinnane i've build an image based on johns PR and for now it still looks good 👍

@timkinnane
Copy link
Contributor

@Sing-Li can you find anyone at RC to take over maintenance of hubot-rocketchat and approve this PR. @JSzaszvari did a fair amount of work last year and would be good to keep it. I actually used this PR as a base for the bBot boilerplate docker too and it's tidy work :D

amirhmoradi added a commit to amirhmoradi/hubot-rocketchat that referenced this pull request Jan 4, 2022
…ile for multistage builds, improve hubot executable, cleanups
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.

Error when I don't need any EXTERNAL_SCRIPTS
6 participants