remove the tmp secret data #11116
remove the tmp secret data #11116
Conversation
|
@csrwng ptal,thanks. |
1 similar comment
|
@csrwng ptal,thanks. |
| @@ -116,7 +116,7 @@ func (c *builderConfig) setupGitEnvironment() ([]string, error) { | |||
| if err != nil { | |||
| return nil, fmt.Errorf("cannot fix source secret permissions: %v", err) | |||
| } | |||
|
|
|||
| defer os.RemoveAll(sourceSecretDir) | |||
There was a problem hiding this comment.
This is not the appropriate place to remove the source secret directory, since environment variables have been set that point to files in this directory so git clone can succeed. It would need to be removed after the source has been cloned.
openshift-bot
added
the
needs-rebase
guangxuli
force-pushed
the
build_controller_jenkins_test
branch
2 times, most recently
from
060b137 to
acd2eeb
Compare
|
@csrwng thanks, I review the code again, yes, the place is not appropriate. it should be removed after the building over, I found we have to add a return value that need to remove, and I think we need remove this directory in contidion that the build is success. so when build fail, we can check the secret files that exist in temp directory. Is it reasonable? |
openshift-bot
removed
the
needs-rebase
|
@csrwng ptal,thanks. |
| return nil, fmt.Errorf("cannot fix source secret permissions: %v", err) | ||
| sourceSecretDir, errSecret = fixSecretPermissions(c.sourceSecretDir) | ||
| if errSecret != nil { | ||
| return sourceSecretDir, nil, fmt.Errorf("cannot fix source secret permissions: %v", errSecret) |
There was a problem hiding this comment.
If an error occurred you should delete the secret directory and then return the error. Otherwise, the secret directory will not get removed (the caller function simply returns the error -- line 151)
| secretsEnv, overrideURL, err := scmAuths.Setup(sourceSecretDir) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cannot setup source secret: %v", err) | ||
| return sourceSecretDir, nil, fmt.Errorf("cannot setup source secret: %v", err) |
guangxuli
force-pushed
the
build_controller_jenkins_test
branch
from
acd2eeb to
7b361d3
Compare
|
@csrwng ptal,thanks. |
|
@guangxuli I'm so sorry, I just realized that the last change I asked you to make is not the right thing to do. If there was an error setting up the secrets, we probably want the directory to remain so we can understand what happened. If you can revert that change, the PR looks good to me. Thank you for your patience. |
|
@csrwng okay, I will revert that. That's my care about before.:) thanks. |
gofmt reslove conflict remove the temp secret when build succ remove defer remove secret directory if an error occurred do not remove the secrets when error occurs
guangxuli
force-pushed
the
build_controller_jenkins_test
branch
from
7b361d3 to
0fe00c9
Compare
|
@csrwng sorry to bother you again, I have revert the code, ptal, thanks. |
|
LGTM |
|
@csrwng I check the error cause, the failure seems not relative to this PR. |
|
#10263 |
|
Evaluated for origin test up to 0fe00c9 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10052/) (Base Commit: dbdc2f8) |
|
[merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10052/) (Image: devenv-rhel7_5179) |
|
Evaluated for origin merge up to 0fe00c9 |
No description provided.