feat(1079): integrate deploy keys features with auth and pipeline routes [5] #2157
Conversation
supra08
changed the title
supra08
changed the title
supra08
force-pushed
the
auto-deploy-keys
branch
from
33a3010
to
5afd5e9
Compare
| @@ -113,6 +120,36 @@ module.exports = () => ({ | |||
|
|
|||
| return defaultCollection.update(); | |||
| }) | |||
| .then(() => { | |||
| if (autoKeysGeneration) { | |||
There was a problem hiding this comment.
This boolean check if scm, such as github has ability to do autoKeysGeneration.
What if such scm, say bitbucket does not have such feature and autoKeysGeneration = true from the payload. How do we handle that case? Would there be:
- add a
catchblock - add another if condition to check both payload sent from ui has
autoKeysGeneration: true, as well as check if given SCM has such ability.
In UI, we can prevent user giving input for if scm does not have autoKeysGeneration feature, but we also need to sanitize and guard on API. I can see some power users would call this API directly.
Just a thought, what do you think?
Note:
From: #1079 (comment)
If we apply this feature to all SCMs: scm-github, scm-bitbucket, then we dont have such issue, just need to catch the errors.
There was a problem hiding this comment.
Yeah I get the point. I didn't take the case of direct API calls.
Catch is definitely needed. And it will be handled by the not implemented promise rejection in scm base.
I am wondering if the 2nd point is needed because if autoKeysGeneration is set to true in the payload, it will be rejected in the addDeployKey method and eventually sent to the catch block.
There was a problem hiding this comment.
Cool. I meant either 1) or 2).
Since handled that case in 1), 2) is no longer necessary
There was a problem hiding this comment.
don't we already have a catch at end of Promise chain ?
plugins/pipelines/create.js
Outdated
| @@ -145,7 +139,8 @@ module.exports = () => ({ | |||
| value: privateDeployKeyB64, | |||
| allowInPR: false | |||
| }); | |||
| }); | |||
| }) | |||
| .catch(err => reply(boom.boomify(err))); | |||
There was a problem hiding this comment.
won't this still execute rest of the Promise chain ?
I thought was this catch is not required, and let the err propagate to the final catch at 4237422#diff-1eb6375f360812e82460d42f926123b1R170
Co-authored-by: Tiffany K <tiffanykyi@gmail.com>
supra08
force-pushed
the
auto-deploy-keys
branch
from
fa54638
to
9f11578
Compare
…tes [5] (screwdriver-cd#2157) * feat(): add autoKeysGeneration key value in auth contexts and pipelines creation * feat(): add tests for autoKeysGeneration in auth contexts and pipeline creation * fix(): fix deploy key secret name * refactor(): change function names to autoDeployKeyGenerationEnabled * refactor(): Update plugins/pipelines/create.js Co-authored-by: Tiffany K <tiffanykyi@gmail.com> * refactor(): Add return statements * cleanup(): Remove commented code * lint(): Add return statement corresponding to arrow function * fix(): Make autoDeployKeyGenerationEnabled to that of non promise function * fix(): Fix secret value in test * fix(): Remove redundant usage of pipelineFactory * refactor(): remove unreachable paths and change variable names * cleanup(): remove straw console logs * fix(): Remove unnecessary get secret test assertion * fix(): Add necessary catch block * fix(): remove unnecessary catch block Co-authored-by: Tiffany K <tiffanykyi@gmail.com>
Context
Fixes #1079
Objective
This PR adds functionality of handling of autoKeysGeneration visibility in the UI and its response to trigger it in the scm-github module.
References
#1079
License
I confirm that this contribution is made under a BSD license and that I have the authority necessary to make this contribution on behalf of its copyright owner.