Skip to content
This repository was archived by the owner on May 6, 2020. It is now read-only.

fix(api): cast DEPLOY_BATCHES and DEPLOY_TIMEOUT to int#1076

Merged
bacongobbler merged 1 commit intodeis:masterfrom
bacongobbler:cast-envvars-to-int
Sep 18, 2016
Merged

fix(api): cast DEPLOY_BATCHES and DEPLOY_TIMEOUT to int#1076
bacongobbler merged 1 commit intodeis:masterfrom
bacongobbler:cast-envvars-to-int

Conversation

@bacongobbler
Copy link
Copy Markdown
Member

@bacongobbler bacongobbler commented Sep 16, 2016

Otherwise it will be a string in the scheduler code and break it.

closes deis/builder#429

@bacongobbler bacongobbler added this to the v2.6 milestone Sep 16, 2016
@bacongobbler bacongobbler self-assigned this Sep 16, 2016
@deis-bot
Copy link
Copy Markdown

@helgi, @kmala and @Joshua-Anderson are potential reviewers of this pull request based on my analysis of git blame information. Thanks @bacongobbler!

@helgi helgi added the LGTM1 label Sep 16, 2016
@helgi
Copy link
Copy Markdown
Contributor

helgi commented Sep 16, 2016

https://github.com/deis/controller/blob/master/rootfs/scheduler/__init__.py#L374 is where some of that was being cast to int thus far - I like yours better

@bacongobbler bacongobbler force-pushed the cast-envvars-to-int branch 2 times, most recently from ed02614 to 7403843 Compare September 16, 2016 00:32
@kmala kmala added the LGTM2 label Sep 16, 2016
@bacongobbler bacongobbler force-pushed the cast-envvars-to-int branch 2 times, most recently from c633671 to 8aea0c0 Compare September 16, 2016 02:42
@bacongobbler
Copy link
Copy Markdown
Member Author

blocked by #1077

@helgi
Copy link
Copy Markdown
Contributor

helgi commented Sep 16, 2016

@bacongobbler the or 0 part worries me - no timeout then? not grace period?

@bacongobbler
Copy link
Copy Markdown
Member Author

I'm going to fix that up, actually. It's because production settings just fetch the envvar but don't supply a default value for these integers, therefore they become the NoneType. Going to shift it around once #1078 is merged.

@helgi
Copy link
Copy Markdown
Contributor

helgi commented Sep 16, 2016

Sounds good - I'm sure there are defaults being set through the scheduler / app model that probably could just be shifted all the way up to the settings file

@bacongobbler bacongobbler force-pushed the cast-envvars-to-int branch 2 times, most recently from 559efe1 to e3ffda8 Compare September 18, 2016 20:44
Otherwise it will be a string in the scheduler code and break it.
@codecov-io
Copy link
Copy Markdown

Current coverage is 87.14% (diff: 100%)

Merging #1076 into master will increase coverage by 0.02%

@@             master      #1076   diff @@
==========================================
  Files            42         42          
  Lines          3601       3601          
  Methods           0          0          
  Messages          0          0          
  Branches        609        609          
==========================================
+ Hits           3137       3138     +1   
+ Misses          307        306     -1   
  Partials        157        157          

Powered by Codecov. Last update b8aa206...1b7c64c

@bacongobbler bacongobbler merged commit ad182b2 into deis:master Sep 18, 2016
@bacongobbler bacongobbler deleted the cast-envvars-to-int branch September 18, 2016 22:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Set DEIS_DEPLOY_TIMEOUT launch Creating build... Error: Unknown Error (400): {"detail":"Can't convert 'int' object to str implicitly"}

6 participants