2017-11-12T03:57:19  *** okurz has quit IRC
2017-11-12T03:58:59  *** okurz has joined #opensuse-admin
2017-11-12T04:18:06  <heroes-bot> PROBLEM: Hosts syslog on monitor.infra.opensuse.org - CRITICAL: Found files older than 600 minutes /var/log/opensuse/hosts//linux.log was last modified on Fri Nov 10 18:09:15 2017 ; See https://monitor.opensuse.org/icinga/cgi-bin/extinfo.cgi?type=2&host=monitor.infra.opensuse.org&service=Hosts%20syslog
2017-11-12T08:46:54  *** cboltz has joined #opensuse-admin
2017-11-12T11:18:36  *** fvogt has joined #opensuse-admin
2017-11-12T12:24:33  *** tigerfoot has joined #opensuse-admin
2017-11-12T13:03:38  <cboltz> tampakrap: a salt highstate gives me   Attempt to authenticate with the salt master failed with timeout error
2017-11-12T13:04:24  <cboltz> any idea why the saltmaster refuses to work? ("it's sunday" is not a valid excuse ;-)
2017-11-12T13:13:06  <tampakrap> cboltz: I shut it down yesterday because I want to deploy carefully after all those big changes we did
2017-11-12T13:13:23  <cboltz> that explains it ;-)
2017-11-12T13:13:41  <tampakrap> I will start it now, but be careful on the output please
2017-11-12T13:13:56  <cboltz> I'll start with test=True ;-)
2017-11-12T13:13:57  <tampakrap> regarding the big test that you mention, if we do it that way then we lose results
2017-11-12T13:14:23  <tampakrap> if more than two scripts fail, we won't get the results of the second unless we fix the first script
2017-11-12T13:15:27  <cboltz> the good thing of "unittest" is that it can run multiple tests (even if some fail) and will report all failures at the end
2017-11-12T13:16:13  <cboltz> by default, it does this for all test functions in one script (which means we'd need to call all tests from one script)
2017-11-12T13:16:56  <tampakrap> how?
2017-11-12T13:17:20  <tampakrap> master is up
2017-11-12T13:19:12  <cboltz> maybe just look at an example
2017-11-12T13:19:42  <cboltz> https://gitlab.com/apparmor/apparmor/tree/master/utils/test - each of the test-*.py scripts there runs lots of tests
2017-11-12T13:20:06  <cboltz> test-aa.py is probably a good example to get started
2017-11-12T13:21:40  <tampakrap> thanks, I'll check it
2017-11-12T13:21:45  <tampakrap> so, something is wrong
2017-11-12T13:22:01  <tampakrap> highstate on saltmaster shows this diff
2017-11-12T13:22:03  <tampakrap>                       -reactor:
2017-11-12T13:22:05  <tampakrap>                       -  - 'salt/fileserver/gitfs/update':
2017-11-12T13:22:07  <tampakrap>                       -    - /srv/reactor/update_fileserver.sls
2017-11-12T13:22:10  <tampakrap> with test=True
2017-11-12T13:23:11  <tampakrap> can you check on your test env if you get the same?
2017-11-12T13:23:55  <tampakrap> ah no this is correct
2017-11-12T13:24:05  <tampakrap> this diff comes from /etc/salt/minion.d/reactor.conf
2017-11-12T13:24:23  <tampakrap> it shouldn't be there at all, it should have been in /etc/salt/master.d/reactor.conf
2017-11-12T13:31:28  <cboltz> the diff on water looks as expected - it will add "tinker panic 0", "server ntp3.infa.o.o iburst" and "restrict ntp3.infra.o.o"
2017-11-12T13:32:56  <tampakrap>           ID: repo-update-oss
2017-11-12T13:32:58  <tampakrap>     Function: pkgrepo.managed
2017-11-12T13:33:00  <tampakrap>       Result: None
2017-11-12T13:33:02  <tampakrap>      Comment: Package repo 'repo-update-oss' will be configured. This may cause pkg states to behave differently than stated if this action is repeated without test=True, due to the differences in the configured repositories.
2017-11-12T13:33:10  <tampakrap> this is with test=True, but without test=True nothing happens
2017-11-12T13:33:17  <tampakrap> what is it supposed to mean? :/
2017-11-12T13:33:37  <cboltz> it means that repos are spamming you with test=True
2017-11-12T13:33:52  <cboltz> no idea why, but I always see this with test=True
2017-11-12T13:34:17  <tampakrap> okay whatever
2017-11-12T13:35:28  <cboltz> on the positive side, if there is a repo change, it really gets done now (there was a salt bug which is solved)
2017-11-12T13:36:12  <tampakrap> true
2017-11-12T13:37:28  <cboltz> Nov 12 13:36:01 riesling ntpd[25624]: restrict 0.0.0.0: KOD does nothing without LIMITED.
2017-11-12T13:37:30  <cboltz> Nov 12 13:36:01 riesling ntpd[25624]: restrict ::: KOD does nothing without LIMITED.
2017-11-12T13:38:00  <cboltz> I'm not a ntpd expert (usually it just works), but it looks like ntpd complains about these options ;-)
2017-11-12T13:49:18  <tampakrap> yep something seems missing
2017-11-12T13:53:38  <tampakrap> cboltz: https://gitlab.infra.opensuse.org/infra/salt/merge_requests/86
2017-11-12T13:55:15  <cboltz> adding the / everywhere is a good idea
2017-11-12T13:55:56  <cboltz> but moving pillar/osfullname/Leap.sls to pillar/osmajorrelease/42.sls is bad for two reasons:
2017-11-12T13:56:29  <cboltz> - you'll need to create a copy once we start to use Leap 15 (thanks to {{ osrelease }}, it's flexible enough for all Leap releases)
2017-11-12T13:56:54  <cboltz> - pillar/osmajorrelease/15.sls will also match SLE 15, and you don't want to have the Leap repos there
2017-11-12T13:57:29  <tampakrap> when sle15 comes, we'll need to refactor a lot
2017-11-12T13:57:38  <tampakrap> due to your second point mostly
2017-11-12T13:57:57  <tampakrap> so I'd like to solve that problem when that time comes
2017-11-12T13:58:16  <cboltz> maybe, but why shold we create another thing we'll need to refactor if the current state has this problem already solved?
2017-11-12T13:58:34  <tampakrap> because it's not, the refactoring will have to happen either way
2017-11-12T14:00:59  <cboltz> still the change you propose means you'll need to refactor more
2017-11-12T14:01:25  <cboltz> and it's an invitation to do copy&paste programming ("cp 42.sls 15.sls" - I'm quite sure the result will work for Leap)
2017-11-12T14:03:34  <tampakrap> why don't we discuss it then?
2017-11-12T14:04:03  <tampakrap> we have a few months for it, let's improve the files based on now, because I am not sure how we're going to solve the problem
2017-11-12T14:04:30  <tampakrap> we may not use osfullname/$file.sls at all, we may put conditionals inside another file
2017-11-12T14:04:32  <cboltz> I prefer to avoid superfluous work, and your change looks like it will create superflous work
2017-11-12T14:05:00  <tampakrap> I had the same discussion with thomic a while ago btw
2017-11-12T14:05:20  <cboltz> I wouldn't be surprised if we end up dropping osmajorrelease/* and use only osfullname/*
2017-11-12T14:05:28  <cboltz> that would be very easy for Leap
2017-11-12T14:05:35  <tampakrap> that's also an assumption though
2017-11-12T14:06:03  <cboltz> for SLE, it might need some conditions on the major version if the modules list changes
2017-11-12T14:07:31  <cboltz> what was thomic's opinion on this?
2017-11-12T14:08:58  <tampakrap> well, he was the one who proposed to put the leap repos to osmajorrelease to be consistent, and we decided to file a ticket at the end and fix the sle15/leap15 issue when the time comes
2017-11-12T14:10:54  <tampakrap> different topic: do we want to run show_highstate on the CI against the github formulas or against our gitlab.i.o.o forks?
2017-11-12T14:11:10  <tampakrap> actually, we can run it against both, and allow failures against the github ones
2017-11-12T14:12:14  <cboltz> running against our version is most important
2017-11-12T14:12:25  <cboltz> but testing both sounds even better ;-)
2017-11-12T14:13:04  <cboltz> bonus points iif you implement a diff view showing "if we update the formulas, the following will change on the minions" - but that's probably easier said then done
2017-11-12T14:13:47  <tampakrap> yeah that's my next plan
2017-11-12T14:13:55  <tampakrap> I want to deprecate the nasty bash script I have there
2017-11-12T14:15:44  <cboltz> I wouldn't call it nasty ;-) but I understand that you want to use your new shiny FORMULAS.yaml file everywhere ;-)
2017-11-12T14:17:19  <cboltz> (which also makes sense to stay consistent - maintaining the yaml file _and_ the bash script is duplicated work)
2017-11-12T14:20:28  *** fvogt has quit IRC
2017-11-12T14:28:05  *** Son_Goku has quit IRC
2017-11-12T14:34:54  *** Son_Goku has joined #opensuse-admin
2017-11-12T15:22:04  *** ldevulder has joined #opensuse-admin
2017-11-12T16:42:34  *** asmorodskyi has joined #opensuse-admin
2017-11-12T16:44:25  *** cboltz has quit IRC
2017-11-12T16:46:37  *** cboltz has joined #opensuse-admin
2017-11-12T17:32:24  <tampakrap> cboltz: do you have a few mins to help me please?
2017-11-12T17:32:34  <tampakrap> I'm stuck on something for almost two hours
2017-11-12T17:32:36  <tampakrap> https://gitlab.infra.opensuse.org/infra/salt/-/jobs/367
2017-11-12T17:33:34  <tampakrap> the line that fails https://gitlab.infra.opensuse.org/infra/salt/blob/tampakrap_get_formulas/bin/get_formulas.py#L23
2017-11-12T17:35:07  <cboltz> boring idea -   mkdir /srv/formula   first?
2017-11-12T17:36:34  <tampakrap> it happens in line 18
2017-11-12T17:36:51  <cboltz> ok
2017-11-12T17:37:08  *** ldevulder has quit IRC
2017-11-12T17:37:57  <tampakrap> I can reproduce locally with tumbleweed and python 3.6
2017-11-12T17:38:04  <tampakrap> the path exists for sure
2017-11-12T17:38:26  <cboltz> how exactly do you call the script?
2017-11-12T17:38:52  <tampakrap> as root, same args as the ci runner
2017-11-12T17:47:27  <cboltz> looks like I found the reason
2017-11-12T17:47:40  <cboltz> after the git clone call, I added
2017-11-12T17:47:44  <cboltz>     import time
2017-11-12T17:47:48  <cboltz>     time sleep(5)
2017-11-12T17:48:18  <cboltz> looks like subprocess.Popen doesn't wait until the command finishes
2017-11-12T17:49:45  <tampakrap> aha
2017-11-12T17:50:24  <tampakrap> so if I do out = subprocess.Popen(...) it should work
2017-11-12T17:51:54  <cboltz> no
2017-11-12T17:52:01  <cboltz> but you can use subprocess.call instead
2017-11-12T17:52:12  <tampakrap> okay
2017-11-12T17:52:34  <cboltz> subprocess.call(['git', 'clone', url, FULL_PATH])
2017-11-12T17:52:49  <cboltz> I'll leave /dev/null'ing the output as exercise for you, dinner is finally ready ;-)
2017-11-12T17:53:35  <tampakrap> works!
2017-11-12T17:53:56  <tampakrap> with stdout=subprocess.PIPE to remove the output
2017-11-12T18:39:25  <tampakrap> cboltz: https://gitlab.infra.opensuse.org/infra/salt/merge_requests/87
2017-11-12T18:59:53  <cboltz> I'd prefer not to hide stderr and use "git $whatever --quiet" instead
2017-11-12T19:01:34  <tampakrap> it exists as global argument?
2017-11-12T19:03:01  <cboltz> no, but it seems to exist for all noisy git commands I checked (clone, pull, fetch)
2017-11-12T19:03:20  <cboltz> note that "git remote" doesn't support --quiet, proably because it isn't noisy
2017-11-12T19:05:33  <tampakrap> okay
2017-11-12T19:06:02  <tampakrap> the thing is that it will be printed, but it won't pass the exit code to the script
2017-11-12T19:06:19  <tampakrap> so better to actually check if the subprocess.call is failing or not
2017-11-12T19:09:16  <cboltz> both is useful - the error message to see why it failed, and the exitcode to actually make the script fail
2017-11-12T19:11:09  <tampakrap> okay
2017-11-12T19:12:55  <cboltz> https://docs.python.org/3/library/subprocess.html#subprocess.check_call
2017-11-12T19:13:23  <cboltz> looks like the most boring solution is   subprocess.run(['git', 'clone', ...], check=True)
2017-11-12T19:13:33  <cboltz> check=True will raise an exception if $? != 0
2017-11-12T19:15:30  <cboltz> oh, and since python 3.3, there's also subprocess.DEVNULL ;-)
2017-11-12T19:18:06  <tampakrap> subprocess.run is not on 3.4
2017-11-12T19:18:59  <cboltz> right, so keep the .call
2017-11-12T19:19:21  <cboltz> or use .check_call which will raise an exception if $? != 0
2017-11-12T19:19:42  <cboltz> BTW:
2017-11-12T19:19:48  <cboltz>     Note Do not use stdout=PIPE or stderr=PIPE with this function. The child process will block if it generates enough output to a pipe to fill up the OS pipe buffer as the pipes are not being read from.
2017-11-12T19:20:24  <tampakrap> give me a min
2017-11-12T19:21:31  <tampakrap> https://gitlab.infra.opensuse.org/infra/salt/commit/9eea5e3f48e4f9192303901b630994f2028f9f6c
2017-11-12T19:21:45  <tampakrap> untested yet
2017-11-12T19:25:29  <tampakrap> not working
2017-11-12T19:26:03  <tampakrap> ah no wait
2017-11-12T19:28:48  <cboltz> that looks too complicated ;-)
2017-11-12T19:29:35  <cboltz> subprocess.call (+ checking return value) or subprocess.check_call should do what we need
2017-11-12T19:29:38  <tampakrap> okay now it works
2017-11-12T19:29:42  <tampakrap> and it also exposed an error
2017-11-12T19:29:47  <tampakrap> https://gitlab.infra.opensuse.org/infra/salt/-/jobs/400
2017-11-12T19:30:24  <tampakrap> I'll try that as well
2017-11-12T19:40:47  <cboltz> nice, https certificate fun...
2017-11-12T19:43:30  <tampakrap> two ways to solve it
2017-11-12T19:43:45  <tampakrap> either we add the openSUSE:infrastructure repo without checking its gpg key
2017-11-12T19:44:25  <tampakrap> or we use git config http.sslVerify "false"
2017-11-12T19:44:55  <tampakrap> we can also pass global env variable GIT_SSL_NO_VERIFY=true
2017-11-12T19:47:02  <tampakrap> your idea works btw see https://gitlab.infra.opensuse.org/infra/salt/commit/7e97fefdc6122221265872df0ce48e121bc0df3c
2017-11-12T19:47:03  <cboltz> IIRC Let's encrypt promised wildcard certs for next year, so we only need one of them as temporary solution
2017-11-12T19:47:16  <tampakrap> and https://gitlab.infra.opensuse.org/infra/salt/pipelines/346
2017-11-12T19:47:19  <cboltz> so choose whatever is easiest to implement - and easiest to remove once we have a valid cert
2017-11-12T19:47:36  <tampakrap> okay
2017-11-12T19:48:03  <cboltz> don't forget to add a "TODO: remove this workaround" note ;-)
2017-11-12T20:06:36  <tampakrap> cboltz: done and tests passed
2017-11-12T20:10:50  <cboltz> nice :-)
2017-11-12T20:12:07  <cboltz> I'm slightly surprised that only "git fetch" needs disabling the certificate check, but well, the tests say everything works ;-)
2017-11-12T20:12:54  <cboltz> can you add a "TODO: get rid of GIT_SSL_NO_VERIFY" note?
2017-11-12T20:13:09  <cboltz> that's the last thing that prevents a +1 from me ;-)
2017-11-12T20:13:29  <tampakrap> `git fetch opensuse` is the only command we do against gitlab.i.o.o
2017-11-12T20:13:43  <tampakrap> everything else is either against github, or local git operation
2017-11-12T20:14:00  <cboltz> ah, right
2017-11-12T20:14:11  <cboltz> you are doing some branch magic ;-)
2017-11-12T20:14:52  <tampakrap> TODO added
2017-11-12T20:16:00  <tampakrap> it will be merged as soon as the pipeline finished
2017-11-12T20:16:04  <tampakrap> finishes*
2017-11-12T20:31:16  *** fvogt has joined #opensuse-admin
2017-11-12T20:35:31  *** asmorodskyi_ has joined #opensuse-admin
2017-11-12T20:38:57  *** asmorodskyi has quit IRC
2017-11-12T20:45:05  <tampakrap> cboltz: https://gitlab.infra.opensuse.org/infra/salt/merge_requests/88
2017-11-12T20:47:39  <cboltz> some better error handling than "pass" would be nice
2017-11-12T20:47:43  <cboltz> but besides that - good idea!
2017-11-12T20:48:17  <tampakrap> we don't need better error handling
2017-11-12T20:48:28  <tampakrap> either there is a list with open prs or there isn't
2017-11-12T20:49:07  <tampakrap> I could put only the open_pull_requests = data['pending'] line in the try block if it makes it more readable
2017-11-12T20:51:23  <cboltz> try blocks should be as small as possible (to avoid that they "accidently" catch more errors than planned), so yes, please do that
2017-11-12T20:51:54  <tampakrap> okay
2017-11-12T20:53:07  <tampakrap> done
2017-11-12T20:57:03  <cboltz> better, but it might fail in funny ways (I didn't test, but what happens if there's no data['pending']? My guess is that open_pull_requests will keep the value of the previous loop, or is undefined if there's no previous loop)
2017-11-12T20:57:05  <cboltz> what about
2017-11-12T20:57:22  <cboltz>     for pull_request in data.get('pending', [])
2017-11-12T20:57:35  <cboltz> and drop the try/except section?
2017-11-12T20:57:39  <cboltz> (untested)
2017-11-12T20:58:32  <tampakrap> ah corect
2017-11-12T20:58:38  <tampakrap> I'll check
2017-11-12T20:58:58  <tampakrap> I could just replace pass with continue
2017-11-12T20:59:43  <cboltz> or you could use data.get, and get rid of the try/except ;-)
2017-11-12T21:00:13  <tampakrap> yep
2017-11-12T21:03:53  <tampakrap> the problem with that approach is that it will do unnessecary github calls
2017-11-12T21:04:03  <tampakrap> the except continue looks the best
2017-11-12T21:05:11  <cboltz>     if data.get('pending'):
2017-11-12T21:05:23  <cboltz>         org = g.get_organization(namespace)
2017-11-12T21:05:32  <cboltz>         for pull_request in data['pending']:
2017-11-12T21:05:37  <cboltz>             ...
2017-11-12T21:06:32  <cboltz> that avoids superfluous github calls _and_ the try/except block
2017-11-12T21:07:36  <tampakrap> okay check it again
2017-11-12T21:07:39  <cboltz> you can also move the "namespace =" and "prefix =" lines into the if section
2017-11-12T21:07:55  <cboltz> they are not really expensive, but still superfluous to call if there's no pending pr
2017-11-12T21:08:07  <tampakrap> I did already, check it
2017-11-12T21:08:37  <cboltz> :-)
2017-11-12T21:08:45  * cboltz should put a stone on the F5 key
2017-11-12T21:11:15  <cboltz> looks good
2017-11-12T21:13:29  <tampakrap> and this solves finally https://progress.opensuse.org/issues/25300
2017-11-12T21:13:38  <tampakrap> I will put it in the commit message
2017-11-12T21:17:03  <cboltz> :-)
2017-11-12T21:17:32  <cboltz> minor nitpicking - to check for pending pull requests, I still need to specify -d
2017-11-12T21:17:45  <tampakrap> yep, I'm on it already
2017-11-12T21:39:52  <tampakrap> cboltz: https://gitlab.infra.opensuse.org/infra/salt/merge_requests/89
2017-11-12T21:43:31  *** asmorodskyi_ has quit IRC
2017-11-12T21:43:36  <cboltz> looks good, but I'd propose a change nevertheless
2017-11-12T21:44:06  <cboltz> move the "if args.pull_request:" check upwards, above everything that needs -d
2017-11-12T21:44:18  <cboltz> you can then check -d once for all remaining options
2017-11-12T21:50:28  <tampakrap> sure
2017-11-12T21:54:28  *** tigerfoot has quit IRC
2017-11-12T22:00:38  <tampakrap> cboltz: f5
2017-11-12T22:04:18  <cboltz> looks good :-)
2017-11-12T22:05:25  <tampakrap> merged!
2017-11-12T22:05:34  <tampakrap> now to unhardcode the remote
2017-11-12T22:36:31  *** fvogt has quit IRC
2017-11-12T23:45:19  *** cboltz has quit IRC