Issue 29825579: Issue #6681 - Add linting for .gitlab-ci.yml (Closed)
|
Created:
July 9, 2018, 11:37 a.m. by tlucas Modified:
July 10, 2018, 4:01 p.m. CC:
mathias Base URL:
https://hg.adblockplus.org/buildtools/file/9a56d76cd951 Visibility:
Public. |
DescriptionIssue #6681 - Add linting for .gitlab-ci.yml
Patch Set 1 #
Total comments: 2
MessagesTotal messages: 8
Patch Set 1: * Add linting for .gitlab-ci.yml files
Hi Tristan, The change makes sense to me generally although it seems like quite some trouble to run this script (I ended up saying "whatever"). I suppose you have tested it in some kind of realistic scenario. I also have a minor proposal there (see below). Cheers, Vasily https://codereview.adblockplus.org/29825579/diff/29825580/build.py File build.py (right): https://codereview.adblockplus.org/29825579/diff/29825580/build.py#newcode401 build.py:401: print urllib2.urlopen(request).read() Could we maybe set the return code based on the results of the linting? Seems like it would be a useful feature. Or is the response too complicated to interpret?
https://codereview.adblockplus.org/29825579/diff/29825580/build.py File build.py (right): https://codereview.adblockplus.org/29825579/diff/29825580/build.py#newcode401 build.py:401: print urllib2.urlopen(request).read() On 2018/07/09 17:59:48, Vasily Kuznetsov wrote: > Could we maybe set the return code based on the results of the linting? Seems > like it would be a useful feature. Or is the response too complicated to > interpret? It's not hard to interpret at all ( on success it returns '{"status":"valid","errors":[]}' ) but i would stick to 0 or 1 as a return code. What do you think?
On 2018/07/10 09:41:35, tlucas wrote: > https://codereview.adblockplus.org/29825579/diff/29825580/build.py > File build.py (right): > > https://codereview.adblockplus.org/29825579/diff/29825580/build.py#newcode401 > build.py:401: print urllib2.urlopen(request).read() > On 2018/07/09 17:59:48, Vasily Kuznetsov wrote: > > Could we maybe set the return code based on the results of the linting? Seems > > like it would be a useful feature. Or is the response too complicated to > > interpret? > > It's not hard to interpret at all ( on success it returns > '{"status":"valid","errors":[]}' ) but i would stick to 0 or 1 as a return code. > What do you think? Yeah I was also thinking 0 if no errors, 1 if there are errors.
On 2018/07/10 14:30:36, Vasily Kuznetsov wrote: > On 2018/07/10 09:41:35, tlucas wrote: > > https://codereview.adblockplus.org/29825579/diff/29825580/build.py > > File build.py (right): > > > > https://codereview.adblockplus.org/29825579/diff/29825580/build.py#newcode401 > > build.py:401: print urllib2.urlopen(request).read() > > On 2018/07/09 17:59:48, Vasily Kuznetsov wrote: > > > Could we maybe set the return code based on the results of the linting? > Seems > > > like it would be a useful feature. Or is the response too complicated to > > > interpret? > > > > It's not hard to interpret at all ( on success it returns > > '{"status":"valid","errors":[]}' ) but i would stick to 0 or 1 as a return > code. > > What do you think? > > Yeah I was also thinking 0 if no errors, 1 if there are errors. I just noticed, we'd have to patch every repository's build.py, which then calls buildtools' build.py, in order to let the return code actually bubble up. I guess that's not really worth it?
On 2018/07/10 15:07:27, tlucas wrote: > On 2018/07/10 14:30:36, Vasily Kuznetsov wrote: > > On 2018/07/10 09:41:35, tlucas wrote: > > > https://codereview.adblockplus.org/29825579/diff/29825580/build.py > > > File build.py (right): > > > > > > > https://codereview.adblockplus.org/29825579/diff/29825580/build.py#newcode401 > > > build.py:401: print urllib2.urlopen(request).read() > > > On 2018/07/09 17:59:48, Vasily Kuznetsov wrote: > > > > Could we maybe set the return code based on the results of the linting? > > Seems > > > > like it would be a useful feature. Or is the response too complicated to > > > > interpret? > > > > > > It's not hard to interpret at all ( on success it returns > > > '{"status":"valid","errors":[]}' ) but i would stick to 0 or 1 as a return > > code. > > > What do you think? > > > > Yeah I was also thinking 0 if no errors, 1 if there are errors. > > I just noticed, we'd have to patch every repository's build.py, which then calls > buildtools' build.py, in order to let the return code actually bubble up. I > guess that's not really worth it? Yeah I'd say not worth it (unless you remove all those scripts and use the same tool to build all repos :P).
On 2018/07/10 15:44:04, Vasily Kuznetsov wrote: > On 2018/07/10 15:07:27, tlucas wrote: > > On 2018/07/10 14:30:36, Vasily Kuznetsov wrote: > > > On 2018/07/10 09:41:35, tlucas wrote: > > > > https://codereview.adblockplus.org/29825579/diff/29825580/build.py > > > > File build.py (right): > > > > > > > > > > https://codereview.adblockplus.org/29825579/diff/29825580/build.py#newcode401 > > > > build.py:401: print urllib2.urlopen(request).read() > > > > On 2018/07/09 17:59:48, Vasily Kuznetsov wrote: > > > > > Could we maybe set the return code based on the results of the linting? > > > Seems > > > > > like it would be a useful feature. Or is the response too complicated to > > > > > interpret? > > > > > > > > It's not hard to interpret at all ( on success it returns > > > > '{"status":"valid","errors":[]}' ) but i would stick to 0 or 1 as a return > > > code. > > > > What do you think? > > > > > > Yeah I was also thinking 0 if no errors, 1 if there are errors. > > > > I just noticed, we'd have to patch every repository's build.py, which then > calls > > buildtools' build.py, in order to let the return code actually bubble up. I > > guess that's not really worth it? > > Yeah I'd say not worth it (unless you remove all those scripts and use the same > tool to build all repos :P). Btw, if this is sorted, I guess it's an LGTM now
LGTM 
|
