Skip to content

don't remove SLAVEPASS from environment by default in buildbot-worker#2132

Merged
tardyp merged 1 commit intobuildbot:masterfrom
rutsky:worker-dont-clean-env
Apr 19, 2016
Merged

don't remove SLAVEPASS from environment by default in buildbot-worker#2132
tardyp merged 1 commit intobuildbot:masterfrom
rutsky:worker-dont-clean-env

Conversation

@rutsky
Copy link
Copy Markdown
Member

@rutsky rutsky commented Apr 18, 2016

These lines were introduced in 75f9ad7 by @pieterlexis.
I don't think these changes are required anymore.

Worker configuration created with buildbot-worker create ... doesn't
use environment variables to set password.

Worker configuration created with
master/contrib/docker/worker/Dockerfile optionally uses environment
variable to specify password, but also has it's own code that clears
environment.

So I don't see any supported configuration that requires clearing of the
environment variables.

If user created configuration that uses environment variable to set
password, he must clear environment variables.

@pieterlexis please comment if you think I'm missing something.

These lines were introduced in 75f9ad7 by @pieterlexis.
I don't think these changes are required anymore.

Worker configuration created with `buildbot-worker create ...` doesn't
use environment variables to set password.

Worker configuration created with
master/contrib/docker/worker/Dockerfile optionally  uses environment
variable to specify password, but also has it's own code that clears
environment.

So I don't see any supported configuration that requires clearing of the
environment variables.

If *user* created configuration that uses environment variable to set
password, *he* must clear environment variables.
@codecov-io
Copy link
Copy Markdown

Current coverage is 84.26%

Merging #2132 into master will not affect coverage as of 075d742

@@            master   #2132   diff @@
======================================
  Files          333     333       
  Stmts        32026   32026       
  Branches         0       0       
  Methods          0       0       
======================================
  Hit          26987   26987       
  Partial          0       0       
  Missed        5039    5039       

Review entire Coverage Diff as of 075d742


Uncovered Suggestions

  1. +0.11% via ...ot/status/builder.py#384...417
  2. +0.09% via ...ot/status/builder.py#456...484
  3. +0.09% via ...dbot/changes/mail.py#480...507
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

rutsky added a commit to rutsky/buildbot that referenced this pull request Apr 19, 2016
Covers PR buildbot#2134, buildbot#2132, buildbot#2105.

Splitting them by lines for these PR will introduce merge conflicts, so
I would prefer to document those changes post factum.
@tardyp
Copy link
Copy Markdown
Member

tardyp commented Apr 19, 2016

I think I remember it was because the pass appeared in some log.
I think it is better to erase at the same place as it is actually used. (which is the dockerfile?)

@rutsky
Copy link
Copy Markdown
Member Author

rutsky commented Apr 19, 2016

@tardyp I think so too. Environment variables are outputted in each build; and I know only one place where SLAVEPASS is used (and renamed): https://github.com/buildbot/buildbot/blob/master/master/contrib/docker/worker/buildbot.tac#L21

@tardyp
Copy link
Copy Markdown
Member

tardyp commented Apr 19, 2016

indeed, git grep confirms it.

@tardyp tardyp merged commit 4b506a6 into buildbot:master Apr 19, 2016
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.

4 participants