Issue 5652679430766592: Issue 2443 - Honour SKIP_DEPENDENCY_UPDATES environment variable (Closed)
|
Created:
May 1, 2015, 10:03 a.m. by kzar Modified:
May 8, 2015, 1:51 p.m. CC:
Felix Dahlke, mathias Visibility:
Public. |
DescriptionIssue 2443 - Honour SKIP_DEPENDENCY_UPDATES environment variable
Patch Set 1 #
Total comments: 11
Patch Set 2 : Ignore falsey values. #
Total comments: 4
Patch Set 3 : Only warn if revision is wrong #
Total comments: 4
Patch Set 4 : Addressed Sebastian's feedback #
Total comments: 4
Patch Set 5 : Addressed further feedback from Sebastian #MessagesTotal messages: 23
Patch Set 1
http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... ensure_dependencies.py:300: if os.environ.get("SKIP_ENSURE_DEPENDENCIES"): This will merely check whether the variable is set a non-empty string. Hence either of the following will still disable dependency updates: SKIP_ENSURE_DEPENDENCIES=0 SKIP_ENSURE_DEPENDENCIES=false Maybe we should rather perform following check: os.environ.get("SKIP_ENSURE_DEPENDENCIES", "").lower() not in ("", "0", "false") http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... ensure_dependencies.py:301: logging.warning("SKIP_ENSURE_DEPENDENCIES environment variable set, skipping.") I think I'd prefer to only show a warning if there is actually a dependency update that isn't performed because this variable being set, in update_repo(): if not resolved_revision or resolved_revision != current_revision: logging.warning('Pending update for repository %s detected! Skipping due to SKIP_ENSURE_DEPENDENCIES :(') return Otherwise people using this variable will quickly stop caring about this warning.
Patch Set 2 : Ignore falsey values. http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... ensure_dependencies.py:300: if os.environ.get("SKIP_ENSURE_DEPENDENCIES"): On 2015/05/04 11:33:01, Sebastian Noack wrote: > This will merely check whether the variable is set a non-empty string. Hence > either of the following will still disable dependency updates: > > SKIP_ENSURE_DEPENDENCIES=0 > SKIP_ENSURE_DEPENDENCIES=false > > Maybe we should rather perform following check: > > os.environ.get("SKIP_ENSURE_DEPENDENCIES", "").lower() not in ("", "0", "false") Well I realised that but sure, Done. http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... ensure_dependencies.py:301: logging.warning("SKIP_ENSURE_DEPENDENCIES environment variable set, skipping.") On 2015/05/04 11:33:01, Sebastian Noack wrote: > I think I'd prefer to only show a warning if there is actually a dependency > update that isn't performed because this variable being set, in update_repo(): > > if not resolved_revision or resolved_revision != current_revision: > logging.warning('Pending update for repository %s detected! Skipping due to > SKIP_ENSURE_DEPENDENCIES :(') > return > > Otherwise people using this variable will quickly stop caring about this > warning. Disagree with this one, I think that people that set this variable should be trusted to know what they're doing. IMHO we should always warn them it's enabled but otherwise assume they really mean what they say. I would not want the script to check for updates even though I have turned it off.
http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... ensure_dependencies.py:301: logging.warning("SKIP_ENSURE_DEPENDENCIES environment variable set, skipping.") On 2015/05/05 13:25:22, kzar wrote: > On 2015/05/04 11:33:01, Sebastian Noack wrote: > > I think I'd prefer to only show a warning if there is actually a dependency > > update that isn't performed because this variable being set, in update_repo(): > > > > if not resolved_revision or resolved_revision != current_revision: > > logging.warning('Pending update for repository %s detected! Skipping due to > > SKIP_ENSURE_DEPENDENCIES :(') > > return > > > > Otherwise people using this variable will quickly stop caring about this > > warning. > > Disagree with this one, I think that people that set this variable should be > trusted to know what they're doing. IMHO we should always warn them it's enabled > but otherwise assume they really mean what they say. I would not want the script > to check for updates even though I have turned it off. There are two situations in which people might use this variable: 1. They are temporarily testing a change in a dependency repo. It's easy to forget to remove that variable afterwards. 2. They prefer to update dependencies manually, setting this variable globally, running ensure_dependency.py only once in a while. However, this is a bad practice, leading to broken and outdated builds and while testing their changes with potentially outdated dependencies they introduce even more issues. So no, people who keep that variable don't know what they are doing. However, regardless for what reason you set this variable, pending updates that are skipped is a useful information. In particular for the second scenario above, it's important to remind them when they have outdated dependencies. But I agree that the name of this variable is slightly misleading then. So maybe you want to rename it to PRESERVE_DEPENDENCIES or DONT_UPDATE_DEPENDENCIES, SKIP_DEPENDENCY_UPDATES or UPDATE_DEPENDENCIES (with inverted values).
http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... ensure_dependencies.py:301: logging.warning("SKIP_ENSURE_DEPENDENCIES environment variable set, skipping.") While I'm not sure I agree with Sebastian's argumentation above, showing a more targeted warning would indeed be beneficial - and it is easy enough to implement. The warning should be moved into the |if resolved_revision != current_revision| branch in the update_repo() function. This way various checks would still run and dependencies would still be checked out if necessary, merely the update would be replaced by a warning. SKIP_DEPENDENCY_UPDATES as variable name is fine with me.
http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... ensure_dependencies.py:301: logging.warning("SKIP_ENSURE_DEPENDENCIES environment variable set, skipping.") On 2015/05/05 14:46:37, Wladimir Palant wrote: > While I'm not sure I agree with Sebastian's argumentation above, showing a more > targeted warning would indeed be beneficial - and it is easy enough to > implement. The warning should be moved into the |if resolved_revision != > current_revision| branch in the update_repo() function. This way various checks > would still run and dependencies would still be checked out if necessary, merely > the update would be replaced by a warning. > > SKIP_DEPENDENCY_UPDATES as variable name is fine with me. Felix / Matze - As you two are the main users/proponents of this feature could you give your thoughts here? (I want to avoid making something that's no longer useful for you guys!)
http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... ensure_dependencies.py:301: logging.warning("SKIP_ENSURE_DEPENDENCIES environment variable set, skipping.") On 2015/05/06 16:23:53, kzar wrote: > On 2015/05/05 14:46:37, Wladimir Palant wrote: > > While I'm not sure I agree with Sebastian's argumentation above, showing a > more > > targeted warning would indeed be beneficial - and it is easy enough to > > implement. The warning should be moved into the |if resolved_revision != > > current_revision| branch in the update_repo() function. This way various > checks > > would still run and dependencies would still be checked out if necessary, > merely > > the update would be replaced by a warning. > > > > SKIP_DEPENDENCY_UPDATES as variable name is fine with me. > > Felix / Matze - As you two are the main users/proponents of this feature could > you give your thoughts here? (I want to avoid making something that's no longer > useful for you guys!) On 2015/05/06 16:23:53, kzar wrote: > http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... > File ensure_dependencies.py (right): > > http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... > ensure_dependencies.py:301: logging.warning("SKIP_ENSURE_DEPENDENCIES > environment variable set, skipping.") > On 2015/05/05 14:46:37, Wladimir Palant wrote: > > While I'm not sure I agree with Sebastian's argumentation above, showing a > more > > targeted warning would indeed be beneficial - and it is easy enough to > > implement. The warning should be moved into the |if resolved_revision != > > current_revision| branch in the update_repo() function. This way various > checks > > would still run and dependencies would still be checked out if necessary, > merely > > the update would be replaced by a warning. > > > > SKIP_DEPENDENCY_UPDATES as variable name is fine with me. > > Felix / Matze - As you two are the main users/proponents of this feature could > you give your thoughts here? (I want to avoid making something that's no longer > useful for you guys!) Regarding the warning: Let's definitely have that - this is non-default behaviour and it needs to be reasonably obvious. However, I'd vote for simply always uttering a warning when the variable is set. It's simpler, and I think it's too magical to still do the pull/fetch when this is supposed to be skipped. Regarding the variable name: Related to whether we should skip the whole thing or only the hg update/git checkout. I really think we should do the former, thus I prefer SKIP_ENSURE_DEPENDENCIES.
Agreed; thus patch-set 2 is just fine.
http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... ensure_dependencies.py:301: logging.warning("SKIP_ENSURE_DEPENDENCIES environment variable set, skipping.") On 2015/05/06 16:31:23, Felix H. Dahlke wrote: > Regarding the warning: Let's definitely have that - this is non-default > behaviour and it needs to be reasonably obvious. Nobody argues about that. We already have consensus on having a warning. ;) > However, I'd vote for simply > always uttering a warning when the variable is set. It's simpler It's not simpler. The complexity is the same. It's merely about were we add the check for that variable in the code. >, and I think > it's too magical to still do the pull/fetch when this is supposed to be skipped. It's not magical/unexpected at all, if we call the variable SKIP_DEPENDENCY_UPDATES rather than SKIP_ENSURE_DEPENDENCIES. Reasons to not immediately return, but performing all checks, merely leaving the dependency repositories untouched, from the previous discussion: * A more targeted warning (raising attention of pending updates) * No warning if it doesn't make a difference (when there are no pending updates) * Larger common code path (might be helpful for testing as well) On 2015/05/06 16:23:53, kzar wrote: > Felix / Matze - As you two are the main users/proponents of this feature could > you give your thoughts here? (I want to avoid making something that's no longer > useful for you guys!) I doubt it will be useless to them if we implement it like I suggested above. In fact nobody brought up any valid argument against it. And since module owner and super reviewer already agreed we don't need a 4th or 5th opinion here.
http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... ensure_dependencies.py:301: logging.warning("SKIP_ENSURE_DEPENDENCIES environment variable set, skipping.") On 2015/05/06 16:59:23, Sebastian Noack wrote: > On 2015/05/06 16:31:23, Felix H. Dahlke wrote: > > Regarding the warning: Let's definitely have that - this is non-default > > behaviour and it needs to be reasonably obvious. > > Nobody argues about that. We already have consensus on having a warning. ;) > > > However, I'd vote for simply > > always uttering a warning when the variable is set. It's simpler > > It's not simpler. The complexity is the same. It's merely about were we add the > check for that variable in the code. > > >, and I think > > it's too magical to still do the pull/fetch when this is supposed to be > skipped. > > It's not magical/unexpected at all, if we call the variable > SKIP_DEPENDENCY_UPDATES rather than SKIP_ENSURE_DEPENDENCIES. It'd be clear if you call it SKIP_DEPENDENCY_HG_UPDATE_OR_GIT_CHECKOUT, "SKIP_DEPENDENCY_UPDATES" is fairly ambiguous IMO, and doesn't really sound like it's going to go ahead and download potentially huge changes. > Reasons to not immediately return, but performing all checks, merely leaving the > dependency repositories untouched, from the previous discussion: > > * A more targeted warning (raising attention of pending updates) > * No warning if it doesn't make a difference (when there are no pending updates) True, but as a potential user of this, I wouldn't mind this warning at all. Something along the lines of: "Warning: SKIP_ENSURE_DEPENDENCIES is set, skipping ensure_dependencies.py" > * Larger common code path (might be helpful for testing as well) We're talking about a way of skipping this script, what do we need a large common code path for then? > On 2015/05/06 16:23:53, kzar wrote: > > Felix / Matze - As you two are the main users/proponents of this feature could > > you give your thoughts here? (I want to avoid making something that's no > longer > > useful for you guys!) > > I doubt it will be useless to them if we implement it like I suggested above. In > fact nobody brought up any valid argument against it. And since module owner and > super reviewer already agreed we don't need a 4th or 5th opinion here. I really don't get the impression you understand the uses cases for this, so why do you think it's not necessary to listen to the people who will actually use this? The whole point of this variable is to skip the magic, not to get a different sort of magic. If you insist on implementing it this way, I suggest we don't implement it at all and stick to messing with the dependencies file locally.
I could insist on naming the variable ENABLE_FHD_MATZE_FOOTGUN but let's just LGTM this to stop pointless arguing. I'll happily assign bug puppies on every bug introduced due to this behavior and I think at five bug puppies we'll go back and fix this properly. http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... ensure_dependencies.py:301: logging.warning("SKIP_ENSURE_DEPENDENCIES environment variable set, skipping.") On 2015/05/06 17:14:57, Felix H. Dahlke wrote: > I really don't get the impression you understand the uses cases for this, so why > do you think it's not necessary to listen to the people who will actually use > this? Mostly because these people never manage to provide a valid argument beyond some hand waving and muttering "magic" ;) Much less describe a use case.
On 2015/05/06 17:40:47, Wladimir Palant wrote: > I could insist on naming the variable ENABLE_FHD_MATZE_FOOTGUN but let's just > LGTM this to stop pointless arguing. I'll happily assign bug puppies on every > bug introduced due to this behavior and I think at five bug puppies we'll go > back and fix this properly. > > http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... > File ensure_dependencies.py (right): > > http://codereview.adblockplus.org/5652679430766592/diff/5629499534213120/ensu... > ensure_dependencies.py:301: logging.warning("SKIP_ENSURE_DEPENDENCIES > environment variable set, skipping.") > On 2015/05/06 17:14:57, Felix H. Dahlke wrote: > > I really don't get the impression you understand the uses cases for this, so > why > > do you think it's not necessary to listen to the people who will actually use > > this? > > Mostly because these people never manage to provide a valid argument beyond some > hand waving and muttering "magic" ;) Much less describe a use case. To be fair, I don't think anyone mentioned a use case that wouldn't be covered by this, so here is some: You simply don't want the build process to do any implicit (potentially huge, we have some big deps) downloads, e.g. because you're on 3G (I am 1-2 times a week).
On 2015/05/06 17:43:40, Felix H. Dahlke wrote: > To be fair, I don't think anyone mentioned a use case that wouldn't be covered > by this, so here is some: You simply don't want the build process to do any > implicit (potentially huge, we have some big deps) downloads, e.g. because > you're on 3G (I am 1-2 times a week). Yes, I already realized that my suggestion above wasn't quite correct - in order to prevent fetching the check should move a bit up. But that's merely an implementation question, the point was that all the check should run but actual actions skipped.
On 2015/05/06 17:43:40, Felix H. Dahlke wrote: > To be fair, I don't think anyone mentioned a use case that wouldn't be covered > by this, so here is some: You simply don't want the build process to do any > implicit (potentially huge, we have some big deps) downloads, e.g. because > you're on 3G (I am 1-2 times a week). We could easily skip both, fetching and updating the repo when SKIP_DEPENDENCY_UPDATES is set, issuing a targeted warning. Fine with me. Actually this is what I initially suggested.
On 2015/05/06 17:55:06, Wladimir Palant wrote: > On 2015/05/06 17:43:40, Felix H. Dahlke wrote: > > To be fair, I don't think anyone mentioned a use case that wouldn't be covered > > by this, so here is some: You simply don't want the build process to do any > > implicit (potentially huge, we have some big deps) downloads, e.g. because > > you're on 3G (I am 1-2 times a week). > > Yes, I already realized that my suggestion above wasn't quite correct - in order > to prevent fetching the check should move a bit up. But that's merely an > implementation question, the point was that all the check should run but actual > actions skipped. If there's no implicit pull/fetch happening then that sounds better than always showing the warning, indeed. (Please remove me and matze as reviewers Dave, we've got automatically added but shouldn't be reviewers here.)
http://codereview.adblockplus.org/5652679430766592/diff/5724160613416960/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5652679430766592/diff/5724160613416960/ensu... ensure_dependencies.py:229: resolved_revision = repo_types[type].get_revision_id(target, revision) For reference, I think the check belongs here: if resolved_revision != current_revision and os.environ.get("SKIP_ENSURE_DEPENDENCIES", "").lower() not in ("", "0", "false"): logging.warning(...) sys.exit() The repository will still be created if it doesn't exist yet - that's typically a one-time thing when the build is created for the first time. I don't see any use case where that would be an issue.
Patch Set 3 : Only warn if revision is wrong http://codereview.adblockplus.org/5652679430766592/diff/5724160613416960/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5652679430766592/diff/5724160613416960/ensu... ensure_dependencies.py:229: resolved_revision = repo_types[type].get_revision_id(target, revision) On 2015/05/06 18:17:32, Wladimir Palant wrote: > For reference, I think the check belongs here: > > if resolved_revision != current_revision and > os.environ.get("SKIP_ENSURE_DEPENDENCIES", "").lower() not in ("", "0", > "false"): > logging.warning(...) > sys.exit() > > The repository will still be created if it doesn't exist yet - that's typically > a one-time thing when the build is created for the first time. I don't see any > use case where that would be an issue. OK I've done this, I have some reservations though. The sys.exit here would cause nested dependencies to be treated differently depending on the checked out revision of the parent dependency, so instead I return. Thing is I'm still not convinced about my latest patchset, it seems weird that the script clones all the dependencies but then doesn't check out the appropriate revision for them. I can't help but feel patch set 2 would have been easier to reason with, but I'm happy to go with whatever the consensus is, I don't expect to use this feature myself.
http://codereview.adblockplus.org/5652679430766592/diff/5724160613416960/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5652679430766592/diff/5724160613416960/ensu... ensure_dependencies.py:229: resolved_revision = repo_types[type].get_revision_id(target, revision) On 2015/05/07 13:02:38, kzar wrote: > On 2015/05/06 18:17:32, Wladimir Palant wrote: > > For reference, I think the check belongs here: > > > > if resolved_revision != current_revision and > > os.environ.get("SKIP_ENSURE_DEPENDENCIES", "").lower() not in ("", "0", > > "false"): > > logging.warning(...) > > sys.exit() > > > > The repository will still be created if it doesn't exist yet - that's > typically > > a one-time thing when the build is created for the first time. I don't see any > > use case where that would be an issue. > > OK I've done this, I have some reservations though. The sys.exit here would > cause nested dependencies to be treated differently depending on the checked out > revision of the parent dependency, so instead I return. Yeah, I'm pretty sure Wladimir meant to return here. > Thing is I'm still not convinced about my latest patchset, it seems weird that > the script clones all the dependencies but then doesn't check out the > appropriate revision for them. I can't help but feel patch set 2 would have been > easier to reason with, but I'm happy to go with whatever the consensus is, I > don't expect to use this feature myself. Patchset 2 certainly was a footgun. I really don't want to start over that discussion. But I see your point about the kinda unexpected behavior with newly cloned repos. It's not a very common case, but if you think it would be better to either perform the update regardless of SKIP_DEPENDENCY_UPDATES for freshly cloned repos, or to skip cloning repos as well, fine with me. http://codereview.adblockplus.org/5652679430766592/diff/5738600293466112/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5652679430766592/diff/5738600293466112/ensu... ensure_dependencies.py:245: if (resolved_revision != current_revision and I think the code structure below below can be simplified like that: if resolved_revision != current_revision: if os.environ.get("SKIP_DEPENDENCY_UPDATES", .. logging.warning(..) return if not not resolved_revision: .. logging.info(..) repo_types[type].update(target, resolved_revision) http://codereview.adblockplus.org/5652679430766592/diff/5738600293466112/ensu... ensure_dependencies.py:246: os.environ.get("SKIP_ENSURE_DEPENDENCIES", "").lower() not in ("", "0", "false")): We agreed on SKIP_DEPENDENCY_UPDATES rather than SKIP_ENSURE_DEPENDENCIES
Patch Set 4 : Addressed Sebastian's feedback http://codereview.adblockplus.org/5652679430766592/diff/5724160613416960/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5652679430766592/diff/5724160613416960/ensu... ensure_dependencies.py:229: resolved_revision = repo_types[type].get_revision_id(target, revision) On 2015/05/07 13:17:19, Sebastian Noack wrote: > On 2015/05/07 13:02:38, kzar wrote: > > On 2015/05/06 18:17:32, Wladimir Palant wrote: > > > For reference, I think the check belongs here: > > > > > > if resolved_revision != current_revision and > > > os.environ.get("SKIP_ENSURE_DEPENDENCIES", "").lower() not in ("", "0", > > > "false"): > > > logging.warning(...) > > > sys.exit() > > > > > > The repository will still be created if it doesn't exist yet - that's > > typically > > > a one-time thing when the build is created for the first time. I don't see > any > > > use case where that would be an issue. > > > > OK I've done this, I have some reservations though. The sys.exit here would > > cause nested dependencies to be treated differently depending on the checked > out > > revision of the parent dependency, so instead I return. > > Yeah, I'm pretty sure Wladimir meant to return here. > > > Thing is I'm still not convinced about my latest patchset, it seems weird that > > the script clones all the dependencies but then doesn't check out the > > appropriate revision for them. I can't help but feel patch set 2 would have > been > > easier to reason with, but I'm happy to go with whatever the consensus is, I > > don't expect to use this feature myself. > > Patchset 2 certainly was a footgun. I really don't want to start over that > discussion. But I see your point about the kinda unexpected behavior with newly > cloned repos. It's not a very common case, but if you think it would be better > to either perform the update regardless of SKIP_DEPENDENCY_UPDATES for freshly > cloned repos, or to skip cloning repos as well, fine with me. Done. http://codereview.adblockplus.org/5652679430766592/diff/5738600293466112/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5652679430766592/diff/5738600293466112/ensu... ensure_dependencies.py:245: if (resolved_revision != current_revision and On 2015/05/07 13:17:19, Sebastian Noack wrote: > I think the code structure below below can be simplified like that: > > if resolved_revision != current_revision: > if os.environ.get("SKIP_DEPENDENCY_UPDATES", .. > logging.warning(..) > return > > if not not resolved_revision: > .. > > logging.info(..) > repo_types[type].update(target, resolved_revision) Done. http://codereview.adblockplus.org/5652679430766592/diff/5738600293466112/ensu... ensure_dependencies.py:246: os.environ.get("SKIP_ENSURE_DEPENDENCIES", "").lower() not in ("", "0", "false")): On 2015/05/07 13:17:19, Sebastian Noack wrote: > We agreed on SKIP_DEPENDENCY_UPDATES rather than SKIP_ENSURE_DEPENDENCIES Done.
http://codereview.adblockplus.org/5652679430766592/diff/5754903989321728/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5652679430766592/diff/5754903989321728/ensu... ensure_dependencies.py:200: def skip_dependency_updates_p(): I'd suggest to make it either a constant rather than a function for simplicity: SKIP_DEPENDENCY_UPDATES = os.environ.get( "SKIP_DEPENDENCY_UPDATES", "" ).lower() not in ("", "0", "false") Or to move the warning into the function as well for less code duplication: def check_skip_dependency_updates(reason): var = "SKIP_DEPENDENCY_UPDATES" if os.environ.get(var, "").lower() in ("", "0", "false"): return False logging.warning("%s environment variable set, %s", var, reason) return True http://codereview.adblockplus.org/5652679430766592/diff/5754903989321728/ensu... ensure_dependencies.py:209: logging.warning("SKIP_DEPENDENCY_UPDATES environment variable set, " Note that you don't need/should format the message yet, but rather pass the values to be inserted as positional arguments, see above.
Patch Set 5 : Addressed further feedback from Sebastian http://codereview.adblockplus.org/5652679430766592/diff/5754903989321728/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5652679430766592/diff/5754903989321728/ensu... ensure_dependencies.py:200: def skip_dependency_updates_p(): On 2015/05/07 14:06:31, Sebastian Noack wrote: > I'd suggest to make it either a constant rather than a function for simplicity: > > SKIP_DEPENDENCY_UPDATES = os.environ.get( > "SKIP_DEPENDENCY_UPDATES", "" > ).lower() not in ("", "0", "false") > > Or to move the warning into the function as well for less code duplication: > > def check_skip_dependency_updates(reason): > var = "SKIP_DEPENDENCY_UPDATES" > if os.environ.get(var, "").lower() in ("", "0", "false"): > return False > > logging.warning("%s environment variable set, %s", var, reason) > return True Done. http://codereview.adblockplus.org/5652679430766592/diff/5754903989321728/ensu... ensure_dependencies.py:209: logging.warning("SKIP_DEPENDENCY_UPDATES environment variable set, " On 2015/05/07 14:06:31, Sebastian Noack wrote: > Note that you don't need/should format the message yet, but rather pass the > values to be inserted as positional arguments, see above. Done.
LGTM
LGTM 
|
