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

feat: add constructor option for redis config #846

Merged
merged 3 commits into from
Feb 7, 2019

Conversation

chrishiestand
Copy link
Contributor

close #845

@chrishiestand chrishiestand requested a review from a team as a code owner February 6, 2019 21:39
@welcome
Copy link

welcome bot commented Feb 6, 2019

Thanks for opening this pull request! A contributor should be by to give feedback soon. In the meantime, please check out the contributing guidelines and explore other ways you can get involved.

@chrishiestand chrishiestand changed the title add constructor option for redis config feat: add constructor option for redis config Feb 6, 2019
@chrishiestand
Copy link
Contributor Author

I've tested this with redis+sentinels and it works.

127.0.0.1:6379> keys *
 1) "b_octokit-write-556121_client_running"
 3) "b_octokit-global-556121_settings"
 4) "b_octokit-notifications-556121_client_num_queued"
 5) "b_octokit-notifications-556121_client_last_seen"
 6) "b_octokit-global-556121_client_last_seen"
 7) "b_octokit-write-556121_client_last_registered"
 8) "b_octokit-global-556121_client_running"
 9) "b_octokit-notifications-556121_client_running"
10) "b_octokit-write-556121_settings"
12) "b_octokit-write-556121_client_last_seen"
13) "b_octokit-notifications-556121_settings"
14) "b_octokit-global-556121_client_last_registered"
15) "b_octokit-global-556121_client_num_queued"
16) "b_octokit-notifications-556121_client_last_registered"
17) "b_octokit-write-556121_client_num_queued"

Copy link

@SGrondin SGrondin left a comment

Choose a reason for hiding this comment

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

Very cool! 😎

Copy link
Contributor

@tcbyrd tcbyrd left a comment

Choose a reason for hiding this comment

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

Good stuff and thanks @chrishiestand! Just one request to update the type for redisConfig, if possible.

src/index.ts Outdated Show resolved Hide resolved
Redis.RedisOptions does not support type string so
we now keep the config object and env string in separate variables
src/index.ts Outdated Show resolved Hide resolved
@tcbyrd tcbyrd merged commit 1c71952 into probot:beta Feb 7, 2019
@welcome
Copy link

welcome bot commented Feb 7, 2019

Thanks for your contribution to probot! 🎉
Congrats!

@probotbot
Copy link

🎉 This PR is included in version 8.0.0-beta.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@chrishiestand
Copy link
Contributor Author

chrishiestand commented Feb 8, 2019

@tcbyrd It might just be something I don't see on my side, but I'm experiencing an issue with the build artifact from this PR. If I build my app using the new git branch from this PR, and run it, data is written to redis+sentinels as I expect. But if I build with npm package probot@8.0.0-beta.6, and run it, no data is written to redis+sentinels (and no errors are emitted). I suspect there is a difference between the shipped npm package and building from the git repo. I could move this problem into an issue so it can be more readily seen by anyone else experiencing it.

Here is the except from my app Dockerfile that shows how I'm building directly from the git branch:

FROM node:10-alpine

# temporary extras RUNs for probot testing
RUN echo installing os dependencies... && \
    apk add --update --no-cache -t build-deps git make python
RUN echo installing node dependencies... && \
    npm --prefix=/app install
RUN echo probot build... && \
    cd /app/node_modules/ && \
    rm -rf probot && \ 
    git clone --single-branch --branch v8.0.0-beta.6@beta https://github.com/probot/probot && \
    cd probot && \
    npm install && \
    npm run build && \
    echo cleaning up... && \
    apk del build-deps && \
    rm -rf /root/.npm /root/.node-gyp

@tcbyrd

This comment has been minimized.

@chrishiestand
Copy link
Contributor Author

Sorry, those were typos in the comment (not in my dockerfile). I fixed them via edit. I am using the git tag v8.0.0-beta.6@beta

@chrishiestand
Copy link
Contributor Author

In both cases, package.json shows

    "probot": "^8.0.0-beta.6",

@tcbyrd
Copy link
Contributor

tcbyrd commented Feb 8, 2019

Just to be sure, does it work if you install from NPM, and just not from the tagged release? I'm not 💯 clear how semantic release cuts the release. Maybe @gr2m would know?

@chrishiestand
Copy link
Contributor Author

Install which version from npm? If you mean 8.0.0-beta.6 then yeah, the problem is that it doesn't write to redis as expected. That seems to be the correct version: https://www.npmjs.com/package/probot/v/8.0.0-beta.6?activeTab=versions

In the case of installing from npm my Dockerfile excerpt is this:

# temporary extras RUNs for probot testing
RUN echo installing os dependencies... && \
    apk add --update --no-cache -t build-deps git make python
RUN echo installing node dependencies... && \
    npm --prefix=/app install && \
    echo cleaning up... && \
    apk del build-deps && \
    rm -rf /root/.npm /root/.node-gyp

gr2m pushed a commit that referenced this pull request Mar 3, 2019
* add constructor option for redis config

close #845
gr2m pushed a commit that referenced this pull request Mar 8, 2019
* add constructor option for redis config

close #845
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.

4 participants