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

Use octokit github client #146

Merged
merged 1 commit into from
Jul 31, 2018
Merged

Use octokit github client #146

merged 1 commit into from
Jul 31, 2018

Conversation

holgerkoser
Copy link
Contributor

@holgerkoser holgerkoser commented Jul 27, 2018

Intended improvements

  • use github octokit client library
  • added acceptance tests for webhook and journal cache
  • tried to follow nodejs best practices and conventions
  • allow configuration of github ca in helm chart

Changes in detail

  • backend/lib/github/octokit.js
    Wrapper around the octokit client library. Adds PageStream class which creates a Readable stream based on pageable github resources. It adds reduce and thru methods to the streams. They allow interception / transform the chunk or to concat all data. Both return a Promise if the stream ended or failed. It also read authentiction from config.
  • backend/lib/github/index.js
    This module exposes those functions needed for issues and comments in our case. They make use of the PageStream for pageable resources.
  • backend/lib/github/webhook.js
    Implements the github webhook route and does JSON body parsing with signature validation using the body-parser verify hook. It handles the github webhook events and updates the cache for issues and comments. In case of an reopended issues it reloads all comments and updates the cache in the background. All webhooks return synchronous.
  • backend/lib/services/journals.js
    This module exposes higher level functions for handling issues and comments. All function starting with load return an empty Promise and do a cache update. The functions starting with get return a stream with a reduce method. This allows to e.g write to the cache.
  • backend/lib/cache/journals.js
    This module now returns a function to create the cache instead of the cache object. This is needed for testing. The event registration is now done by the EventEmitter class.
  • backend/lib/io.js
    Extracted some functions to make it more modular. In case of journalNsp all comments for each issue are fetch sequentially in order to handle errors individually.
  • charts/gardener-dashboard/templates/configmap.yaml
    Render ca file content in JSON format for oidc and github ca.

Which issue(s) this PR fixes:
Fixes #145

Release note:

[Journal] New optional configuration value in helm chart of gardener-dashboard for github certificate authority

configmap.yaml example:

...
gitHub:
  apiUrl: https://api.foo-github.com
  ca: |-
    -----BEGIN CERTIFICATE-----
    Li4u
    -----END CERTIFICATE-----

repository: 'journal-dev',
webhookSecret: '776562686f6f6b536563726574',
authentication: {
token: '746f6b656e'
Copy link
Contributor

Choose a reason for hiding this comment

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

776562686f6f6b536563726574 === hexEncode('webhookSecret')
746f6b656e === hexEncode('token')
-> these are just dummy values used for unit tests, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If have changed it to:

  webhookSecret: hexEncode('webhookSecret'),
  authentication: {
    token: hexEncode('token')

@@ -1,6 +1,7 @@
port: 3030
logLevel: info
logFormat: text
apiServerUrl: https://minikube:8443
Copy link
Contributor

Choose a reason for hiding this comment

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

is this still used? as we have the same value in backend/lib/config/gardener.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed it in backend/lib/config/gardener.js to

  apiServerUrl: 'https://apiserver:8443',

single webhook file should be sufficient

Added first journal acceptance test

Some more github acceptance test

Handle error for load issue comments

acceptance test for github comments

update dependencies

More tests for octokit

Add `es6-error` license

Allow github and oidc ca in configmap

Remove trailing slash in githubUrl

use for loop instead of lodash forEach
to await inner inner async call before flush emitter

remove defaults for org and repo

only do a time safe equal comparison

fetch shoots parallel
Copy link
Contributor

@petersutter petersutter left a comment

Choose a reason for hiding this comment

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

lgtm

@petersutter petersutter merged commit c4ecbf3 into master Jul 31, 2018
@petersutter petersutter deleted the journals branch July 31, 2018 14:50
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.

Use github octokit client library
2 participants