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

ARC-964 create githubapps table #1185

Merged
merged 7 commits into from
May 13, 2022
Merged

ARC-964 create githubapps table #1185

merged 7 commits into from
May 13, 2022

Conversation

Harminder84
Copy link
Contributor

No description provided.

@Harminder84 Harminder84 changed the title Arc 964 githubapps table Arc 964 create githubapps table May 10, 2022
},
secrets: {
allowNull: false,
type: Sequelize.BLOB
Copy link
Contributor Author

Choose a reason for hiding this comment

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

githubClientSecret, webhookSecret and privateKey will be stored in secrets vault.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean with "secrets vault"? Does that mean they will be stored in this secrets blob, but encrypted?

Copy link
Contributor

Choose a reason for hiding this comment

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

Blob is non-encrypted. DB is encrypted at rest. We need to be careful how we name things since we have Github Client Secret and Github Webhook Secret. We also need to go over how we'll be listening to webhooks for each github enterprise instance as we need listening code for each one or have a way to decipher every event by iterating through all github instances to see which webhook secret will work with the event, which I'm not sure if actually scalable.

We need to brainstorm some solutions for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to store githubClientSecret, webhookSecret and privateKey as encrypted values. The idea here is to put all these values in a secure vault. The vault will be stored as blob in secrets column. The same pattern is being used in installations table with sequelize-encrypted library.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah right, didn't know that was the idea without he model being set

@mboudreau mboudreau changed the title Arc 964 create githubapps table ARC-964 create githubapps table May 10, 2022
@mboudreau
Copy link
Contributor

Don't we want to include the model as part of this? Also, how are we adding the current staging/prod apps to this DB?

@Harminder84 Harminder84 requested a review from thombergs May 10, 2022 02:54
Comment on lines 16 to 19
githubApiBaseUrl: {
allowNull: false,
type: Sequelize.STRING
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this as we can figure out the API url based on if it's a ghe instance or not

Comment on lines 20 to 24
githubServerType: {
allowNull: false,
type: Sequelize.ENUM("cloud", "ghe")

},
Copy link
Contributor

Choose a reason for hiding this comment

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

if we only have 2 choices, we could just make this a boolean

@Harminder84 Harminder84 marked this pull request as draft May 10, 2022 06:48
@Harminder84 Harminder84 marked this pull request as ready for review May 12, 2022 02:39
@Harminder84 Harminder84 requested a review from mboudreau May 12, 2022 02:39
Comment on lines 13 to 16
uuid: {
type: DataTypes.STRING,
unique: true
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that sequelize has a UUID datatype and can create a default value on model creation. Also, you need to set the allowNull: false here.

uuid: {
    type: DataTypes.UUID,
    defaultValue: Sequelize.UUIDV4,
   unique: true,
   allowNull: false
}

@Harminder84 Harminder84 requested a review from mboudreau May 13, 2022 00:46
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.

3 participants