> On Sept. 29, 2017, 5:38 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/app/AppModule.java > > Lines 108 (patched) > > <https://reviews.apache.org/r/62692/diff/1/?file=1839853#file1839853line108> > > > > I would find this easier to read and more flexible: > > > > ``` > > -allowed_job_environments=prod,devel,test,^staging\d+* > > ``` > > > > Then you can do: > > ``` > > -allowed_job_environments=.* > > ``` > > Mauricio Garavaglia wrote: > I'd expect \.* to be the default then, right? > > Bill Farner wrote: > If starting from scratch, yes. In this case, it's more friendly to > prevent surprises by maintaining existing behavior by default. > > Mauricio Garavaglia wrote: > The existing behavior is that the scheduler API ignores whatever > environment comes in, but the CLI restricts it to the list there. Are you > suggesting that now the default, for the API, should be to only allow > 'prod,devel,test,^staging\\d+'? How is that maintaining existing behavior by > default? > > Bill Farner wrote: > My mistake! I believed the scheduler was already restricting this, but i > stand corrected. I support your suggestion for `".*"` as the default. > > Stephan Erb wrote: > I am not convinced we would need to enforce backwards compatibility here. > > Most Aurora users will use the default client and would thus not be > affected if we use the same enforcement at the scheduler side. For those > clusers with custom clients, I think it is OK to ask the operator to > add/update the scheduler flag when deploying the latest scheduler version. > Yes, it is a breaking change but at least one that is very easy to handle if > we call it out in the RELEASE-NOTES.md. > > Mauricio Garavaglia wrote: > Sure, the default can be kept the same. I still found the whole > environment validation extremely arbitrary, with no clear implication for the > cluster operator, and as such is not clear what's its purpose other than > preserve an old behavior. > Anyway, regarding the validation format, is that something like a comma > separated list of Regexps? or is that just a Regexp? > > Stephan Erb wrote: > I believe having just one regex would be the most flexible: > `prod,devel,test,^staging\d+*` could be written as > `prod|devel|test|^staging\d+*`
A single regexp can do an OR of patterns, so we don't need a comma separated list. One might be tempted to attempt a more "user-friendly" format but regexp is a pretty standard construct and I would suggest not re-inventing the wheel. Yet, I don't have a strong opinion and would like to see this and 319 closed quickly :-) - Mohit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62692/#review186700 ----------------------------------------------------------- On Oct. 1, 2017, 3:36 p.m., Mauricio Garavaglia wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62692/ > ----------------------------------------------------------- > > (Updated Oct. 1, 2017, 3:36 p.m.) > > > Review request for Aurora and Stephan Erb. > > > Repository: aurora > > > Description > ------- > > Moves the job environment validation to the scheduler, which can be enabled > with the scheduler require_predefined_environments flag. This allows to have > a consistent behavior when using the CLI and the API. In order to preserve > backward compatibility, the validation is kept in the CLI and for the API it > needs to be manually enabled in the scheduler. > > > Diffs > ----- > > src/main/java/org/apache/aurora/scheduler/app/AppModule.java > 081dff2bda626f01ba222628b8d7d8afebbb0004 > src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java > 186fa1b3a4780c0536fb486d50a33133258110cd > > src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java > 754fde0fdc976b673d78ae15d8ccd8c85b792373 > src/main/python/apache/aurora/client/config.py > 70c2c980309e18de576b251087cdfea00ac06b75 > > src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java > 50d7499f4332a3feb0e2301cb707f2cea6bb2e98 > src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java > 6b4b17f8dafd5c2d751dcda3080b476335f8aec0 > > > Diff: https://reviews.apache.org/r/62692/diff/3/ > > > Testing > ------- > > > Thanks, > > Mauricio Garavaglia > >
