Stroybook auto deployment on UI Pull requests

Development
1

Hi all,

As you all know (hopefully :slight_smile:) we have a playground & documentation tool - our storybook
we observed that over time our storybook breaks over and over, therefore it would be great to have auto deployment on each UI pull request.

I’ve created a PR for this, and I took inspiration from patternfly-react
This PR works with commenting API via personal token, which IMHO works well - we can use our bot user or a other user for it (in order to overcome the rate limit issue).
The other option is to use github statuses API

Storybook meant to serve developers and designers, therefore I think a commenting mechanism would be better - it is more visible on a PR page, and basically it is a part of the review stage and not part of our CI.

any thoughts or alternative option ?

2 Likes
2

Thanks for bringing this up. If we use commenting API, under what conditions the bot will add new comment? The status seems as a good fit from the first glance, is there some downside to this approach?

3

I was thinking that the bot user will add a comment only once - when the pr is published
but the surge deployment will continue on each push (the link still be the same - based on the pr number)

1 Like
4

Thanks, I was happy to see moving the bot commenting on misaligned commit message to check API as it’s annoying to get 2 emails for just opening 1 PR, but I can live with this :slight_smile: If that’s one extra email per PR, that’s fine by me. Is there a downside of the other option you mentioned - statuses API? The upside is, it would eliminate this email/notification right?

5

the bot will only comment on UI PR’s right? I think its fine to get it via a comment/email.

What else do you need to get going?

6

Yeah, it only adds the comment if webpack/ or package.json was modified.

I think this will be a great addition for new features and components as well because it would give UX an easy way to test things out.

1 Like
7

As I said, I don’t mind 1 email that much, I’m just curious what’s the downside of using status API that was suggested in the first post as an alternative. If that’s way more work, that’s acceptable argument, I just didn’t get any answer so far. @amirfefer did you investigate this path?

8

Currently this isn’t implemented in the PR. The current code comments on every push. This is why I suggested the status API.

9

I believe @amirfefer mentioned that it wont add a comment if its already there? so its still one commit per PR.

1 Like
10

Travis runs on every push (and manual restarts). I don’t see any code in that PR which checks if there’s already a comment.

Another aspect that @TimoGoebel kindly pointed out is that on every PR it makes the credentials available. That means we can consider those credentials compromised by default. This doesn’t have to be a problem, but it does mean we can’t reuse any existing accounts that we care about.

11

The important thing to note is that there has been malicious code on e.g. npmjs.org that scans for environment variables with credentials and uploads these credentials to some dark parts of the internet. This is why I believe it’s important to separate the CI steps as much as possible (always using a clean environment, we actually do bad job on our jenkins instances there) and removing any credentials for ci steps that don’t need them.

branches:
  only:
    - master
env:
  global:
  - secure: 123
  - secure: 123

stages:
  - test
  - name: deploy
    if: branch = master && type = push
jobs:
  include:
    - stage: test
      env:
        - AWS_ACCESS_KEY_ID= AWS_SECRET_ACCESS_KEY=
      script:
        - npm run lint
        - npm test
        - npm run build
    - stage: deploy
      script:
        - npm run build
        - npm run upload
12

The PR uses a surge token and a limited access token with only commenting, doesn’t seem as an extreme risk IMO

1 Like
13

And those tokens are manageable from Github UI so you can list them and remove some if needed.

14

@Marek_Hulan @TimoGoebel @ekohl are there any other concerns here or can we move forward? To me the security issue is a non-issue considering anyone can comment on foreman PRs.

15

I would love seeing this go forward rather then get stuck mid air… can we reach a resolution?

3 Likes
16

@Walden: I’m good. If the token has limited permissions, I wouldn’t worry too much about this.

17

As long as we’re not reusing any accounts that we actually rely on, I’m fine with proceeding.

I left inline comments on the implementation details where https://github.com/theforeman/foreman/pull/6828/files#r293768313 is the one that deserves at least a response.

18

I didn’t get answer to my question. But I won’t block anything. Next steps are addressing comments in the PR that have been raised 10 days ago.