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

Decentralized secret templates #1621

Merged
merged 4 commits into from
Nov 8, 2019

Conversation

jeanregisser
Copy link
Contributor

@jeanregisser jeanregisser commented Nov 6, 2019

Description

Following @martinvol's comment, I wanted to use the existing template mechanism to place the secrets.json for blockchain-api.
I noticed however that all templates were in the same folder and secrets.json was already used for the mobile project.

I took this as an opportunity to decentralize this a little bit by using a filename convention.
Whenever you need a template for some encrypted file listed in scripts/key_placer.sh, add it next to the xxx.enc file.

For instance if you have packages/my_project/secrets.json listed in scripts/key_placer.sh, you can create packages/my_project/secrets.json.template with the public template content you want and it will be automatically placed if you don't have access to decryption keys.

Let me know what you think.

Tested

Ran yarn keys:decrypt with and without having access to our keyring.
Templates are correctly used when I don't have access to our keyring.

Other changes

N/A

Backwards compatibility

Yes

By using a naming convention for templates, We don't need to maintain the list of templates and we can have multiple templates with the same name but in different locations
@@ -20,7 +20,6 @@
"report-coverage": "yarn run lerna run test-coverage",
"test:watch": "node node_modules/jest/bin/jest.js --watch",
"postinstall": "yarn run lerna run postinstall && patch-package && yarn keys:decrypt",
"preinstall": "bash scripts/create_key_templates.sh",
Copy link
Contributor

@martinvol martinvol Nov 7, 2019

Choose a reason for hiding this comment

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

Not really sure if this is a good practice, but the reason this is in the preinstall is in case some dependency needs this for yarning or build. In case what I just said doesn't make any sense, then just 🚢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It totally makes sense and I was also concerned by this when I made the change.
I think it's ok for now and we can revisit if it causes an issue later.

@martinvol martinvol assigned jeanregisser and unassigned martinvol Nov 7, 2019
@codecov
Copy link

codecov bot commented Nov 8, 2019

Codecov Report

Merging #1621 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1621   +/-   ##
=======================================
  Coverage   74.15%   74.15%           
=======================================
  Files         287      287           
  Lines        7652     7652           
  Branches      667      667           
=======================================
  Hits         5674     5674           
  Misses       1865     1865           
  Partials      113      113
Flag Coverage Δ
#mobile 74.15% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b1de93...8435faa. Read the comment docs.

@jeanregisser jeanregisser added the automerge Have PR merge automatically when checks pass label Nov 8, 2019
@celo-ci-bot-user celo-ci-bot-user merged commit dbc9f28 into master Nov 8, 2019
@celo-ci-bot-user celo-ci-bot-user deleted the jeanregisser/better-secrets-template branch November 8, 2019 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants