Issue 29875559: Issue 6651,6220 - add gitlab-ci config (Closed)
|
Created:
Sept. 5, 2018, 12:29 p.m. by sergei Modified:
Sept. 7, 2018, 9:12 a.m. Base URL:
https://github.com/adblockplus/adblockpluscore.git@3fc393a609971e48a4f2e664800da1444d75d970 Visibility:
Public. |
Description# this change depends on the availability of eyeo gitlab-runner
# the tag will likely be adjusted
# please use https://gitlab.com/eyeo/adblockplus/adblockpluscore/merge_requests/1 to check the CI status of this change and readiness for review
Patch Set 1 #
Total comments: 4
MessagesTotal messages: 14
https://codereview.adblockplus.org/29875559/diff/29875560/.gitlab-ci.yml File .gitlab-ci.yml (right): https://codereview.adblockplus.org/29875559/diff/29875560/.gitlab-ci.yml#newc... .gitlab-ci.yml:19: tags: You said the tag will likely be adjusted - keep in mind, that changing a tag here will end up in the necessity to re-provision the runners. I suggest to settle on a tag now, before the runners are provisioned (also keep in mind, the provisioning of the runners is now semi-blocked until we are sure which tags you will use (or none))
https://codereview.adblockplus.org/29875559/diff/29875560/.gitlab-ci.yml File .gitlab-ci.yml (right): https://codereview.adblockplus.org/29875559/diff/29875560/.gitlab-ci.yml#newc... .gitlab-ci.yml:19: tags: On 2018/09/05 12:45:35, tlucas wrote: > You said the tag will likely be adjusted - keep in mind, that changing a tag > here will end up in the necessity to re-provision the runners. I suggest to > settle on a tag now, before the runners are provisioned (also keep in mind, the > provisioning of the runners is now semi-blocked until we are sure which tags you > will use (or none)) I would ask dev-ops team to make the decision regarding tags, I can only contribute my opinion there. As soon as the decision is made it will be reflected firstly on gitlab in order to check that the everything works and then in this codereview to let other to review it. So firstly runners then CI config, in this case there will be no need to re-provision runners.
I wonder whether we could split this up into multiple jobs like we are doing for adblockpluschrome (see https://codereview.adblockplus.org/29867566/). For adblockpluscore, this would probably look like this: * One job for the tests running on Node.js. * One job for the tests running on Firefox. * One job for the tests running on Chromium. * One job for ESLint.
On 2018/09/05 12:49:55, Sebastian Noack wrote: > I wonder whether we could split this up into multiple jobs like we are doing > for adblockpluschrome (see https://codereview.adblockplus.org/29867566/). > For adblockpluscore, this would probably look like this: > > * One job for the tests running on Node.js. > * One job for the tests running on Firefox. > * One job for the tests running on Chromium. > * One job for ESLint. We definitely could - but IMHO that should be in a different issue, since this merely translates the existing .travis.yml from https://github.com/adblockplus/adblockpluscore/blob/master/.travis.yml .
On 2018/09/05 16:22:23, tlucas wrote: > On 2018/09/05 12:49:55, Sebastian Noack wrote: > > I wonder whether we could split this up into multiple jobs like we are doing > > for adblockpluschrome (see https://codereview.adblockplus.org/29867566/). > > For adblockpluscore, this would probably look like this: > > > > * One job for the tests running on Node.js. > > * One job for the tests running on Firefox. > > * One job for the tests running on Chromium. > > * One job for ESLint. > > We definitely could - but IMHO that should be in a different issue, since this > merely translates the existing .travis.yml from > https://github.com/adblockplus/adblockpluscore/blob/master/.travis.yml . Then we should do it (IMO). Fine with me doing it in a separate issue.
Not really my area, so if it looks OK to y'all it looks good to me.
LGTM remember to delay the push until our runners are running, i'll let you know https://codereview.adblockplus.org/29875559/diff/29875560/.gitlab-ci.yml File .gitlab-ci.yml (right): https://codereview.adblockplus.org/29875559/diff/29875560/.gitlab-ci.yml#newc... .gitlab-ci.yml:19: tags: On 2018/09/05 12:49:47, sergei wrote: > On 2018/09/05 12:45:35, tlucas wrote: > > You said the tag will likely be adjusted - keep in mind, that changing a tag > > here will end up in the necessity to re-provision the runners. I suggest to > > settle on a tag now, before the runners are provisioned (also keep in mind, > the > > provisioning of the runners is now semi-blocked until we are sure which tags > you > > will use (or none)) > > I would ask dev-ops team to make the decision regarding tags, I can only > contribute my opinion there. As soon as the decision is made it will be > reflected firstly on gitlab in order to check that the everything works and then > in this codereview to let other to review it. So firstly runners then CI config, > in this case there will be no need to re-provision runners. I suggest to not bother dev-ops about this decision, FWIW i expect them to be indifferent about the tags ;) And since i'll likely be asked what you ask them about this, i might as well just answer directly: let's stick to nodejs for this, since it's nodejs running here.
On 2018/09/05 16:26:49, Sebastian Noack wrote: > On 2018/09/05 16:22:23, tlucas wrote: > > On 2018/09/05 12:49:55, Sebastian Noack wrote: > > > I wonder whether we could split this up into multiple jobs like we are doing > > > for adblockpluschrome (see https://codereview.adblockplus.org/29867566/). > > > For adblockpluscore, this would probably look like this: > > > > > > * One job for the tests running on Node.js. > > > * One job for the tests running on Firefox. > > > * One job for the tests running on Chromium. > > > * One job for ESLint. > > > > We definitely could - but IMHO that should be in a different issue, since this > > merely translates the existing .travis.yml from > > https://github.com/adblockplus/adblockpluscore/blob/master/.travis.yml . > > Then we should do it (IMO). Fine with me doing it in a separate issue. IMO it makes sense to split it into several jobs. TLucas, will the job run sequentially or in parallel? If they will run sequentially then I would propose to keep it simple for present and split later, mainly in order to reduce the waiting time. Regarding the latter I assume the following workflow: work-work, push to gitlab and wait for finishing of CI, if the status is successful then send changes to rietveld. > remember to delay the push until our runners are running, i'll let you know Sure, I will follow the workflow above, firstly obtain a successful CI status and then we can continue with the codereview, e.g. land it if no changes are required. Additionally, I would post-pone any changes here until the runner is running. On 2018/09/06 07:14:13, tlucas wrote: > ... > I suggest to not bother dev-ops about this decision, FWIW i expect them to be > indifferent about the tags ;) Well, we need to find out who will maintain the set of runners and their configurations. > And since i'll likely be asked what you ask them about this, i might as well just answer directly: let's stick to nodejs for this, since it's nodejs running here. BTW, should that runner also include tags indicating that it's capable to run chrome with xvfb and headless firefox? I think it can be very useful and in this case the codereview should be accordingly updated. Just for convenience here is a couple of links https://docs.gitlab.com/ee/ci/yaml/#tags https://docs.gitlab.com/ee/ci/runners/#using-tags
On 2018/09/06 09:08:50, sergei wrote: > On 2018/09/05 16:26:49, Sebastian Noack wrote: > > On 2018/09/05 16:22:23, tlucas wrote: > > > On 2018/09/05 12:49:55, Sebastian Noack wrote: > > > > I wonder whether we could split this up into multiple jobs like we are > doing > > > > for adblockpluschrome (see https://codereview.adblockplus.org/29867566/). > > > > For adblockpluscore, this would probably look like this: > > > > > > > > * One job for the tests running on Node.js. > > > > * One job for the tests running on Firefox. > > > > * One job for the tests running on Chromium. > > > > * One job for ESLint. > > > > > > We definitely could - but IMHO that should be in a different issue, since > this > > > merely translates the existing .travis.yml from > > > https://github.com/adblockplus/adblockpluscore/blob/master/.travis.yml . > > > > Then we should do it (IMO). Fine with me doing it in a separate issue. > > IMO it makes sense to split it into several jobs. TLucas, will the job run > sequentially or in parallel? If they will run sequentially then I would propose > to keep it simple for present and split later, mainly in order to reduce the > waiting time. Regarding the latter I assume the following workflow: work-work, > push to gitlab and wait for finishing of CI, if the status is successful then > send changes to rietveld. One runner for one project can only run one job (that means, since we *currently* only plan one runner for core, all jobs for core would run sequentially.) > > > remember to delay the push until our runners are running, i'll let you know > Sure, I will follow the workflow above, firstly obtain a successful CI status > and then we can continue with the codereview, e.g. land it if no changes are > required. Additionally, I would post-pone any changes here until the runner is > running. > > > On 2018/09/06 07:14:13, tlucas wrote: > > ... > > I suggest to not bother dev-ops about this decision, FWIW i expect them to be > > indifferent about the tags ;) > > Well, we need to find out who will maintain the set of runners and their > configurations. I am a maintainer of the roles for provisioning these runners (i.e. the setup), but not the configuration. If you you want to go that path, go ahead - but please do so ASAP. FWIW that tag-discussion is currently a blocker. > > > And since i'll likely be asked what you ask them about this, i might as well > just answer directly: let's stick to nodejs for this, since it's nodejs running > here. > > BTW, should that runner also include tags indicating that it's capable to run > chrome with xvfb and headless firefox? I think it can be very useful and in this > case the codereview should be accordingly updated. > Just for convenience here is a couple of links > https://docs.gitlab.com/ee/ci/yaml/#tags > https://docs.gitlab.com/ee/ci/runners/#using-tags We will, for starters, have one machine, where everything that core and adblockpluschrome need to be tested is installed. Adding tags and relying on tags would currently strip flexibility from this setup. FWIW this also means that core's configuration wouldn't need a tag at all for now - but adding one will create the need for consistency between the runner's config (@ops) and the CI-config (@core). E.g. adblockpluschrome's two runners will run with "--run-untagged"
OK, since it has successfully passed and the job execution is serialized I would propose to land it as is. Are there any objections?
On 2018/09/06 19:13:10, sergei wrote: > OK, since it has successfully passed and the job execution is serialized I would > propose to land it as is. > Are there any objections? No objection from me if it does what it's supposed to do.
https://codereview.adblockplus.org/29875559/diff/29875560/.gitlab-ci.yml File .gitlab-ci.yml (right): https://codereview.adblockplus.org/29875559/diff/29875560/.gitlab-ci.yml#newc... .gitlab-ci.yml:19: tags: As far as I understand, tags serve to match jobs with runners. So if we'd have a cluster of runners, this would tell the CI to only run this job on a runner that is tagged "nodejs". However, we only have one runner at the moment, and once we add more runners, we don't know yet, how we will tag those. So (unless I misunderstood something), I wouldn't configure any tags for now (which would also be consistent with the configuration we have for adblockpluschrome).
On 2018/09/06 19:36:27, Sebastian Noack wrote: > https://codereview.adblockplus.org/29875559/diff/29875560/.gitlab-ci.yml > File .gitlab-ci.yml (right): > > https://codereview.adblockplus.org/29875559/diff/29875560/.gitlab-ci.yml#newc... > .gitlab-ci.yml:19: tags: > As far as I understand, tags serve to match jobs with runners. So if we'd have a > cluster of runners, this would tell the CI to only run this job on a runner that > is tagged "nodejs". However, we only have one runner at the moment, and once we > add more runners, we don't know yet, how we will tag those. So (unless I > misunderstood something), I wouldn't configure any tags for now (which would > also be consistent with the configuration we have for adblockpluschrome). You understood everything correctly - but there's the following situation right now: There is exactly 1 runner registered for https://gitlab.com/eyeo/adblockplus/adblockpluscore/, configured with tags "eyeo", "core" (those tags were used in developement iirc) and "nodejs" (from this very review). leaving this patch as it is will hence work perfectly fine - however, the runner is not configured to run untagged jobs (like those in adblockpluschrome are), so (that's what just happened in #adblockplus) tagged jobs get stuck. We could change that - but those that know from the top of their head how AND have direct access (i.e. not me) are in vacation afaict. Meaning: i'd leave this as it is for now, FWIW "nodejs" is not wrong. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
