Re: Adding CI to our tree

From: Melanie Plageman <melanieplageman(at)gmail(dot)com>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Andrew Dunstan <andrew(at)dunslane(dot)net>
Subject: Re: Adding CI to our tree
Date: 2021-11-19 22:17:44
Message-ID: CAAKRu_bBCzU-WWKJw2TBWixnV+LU6h8jDF_XuVk+HfxPvMnmbg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

I reviewed the first patch in this set
(v2-0001-ci-Add-CI-for-FreeBSD-Linux-MacOS-and-Windows-uti.patch).

For the README, I found the instructions very clear. My only concern is
that the cirrus-ci UI will change and the instructions on how to enable
cirrus-ci on a repository will not be accessible in the same way in the
future.
That being said, I found your instructions easier to follow than those
on [1].
Perhaps it is better to wait until it becomes a problem and then, at
that point, change the README to guide people to the quickstart link.

I have attached a patch which does a small refactor using a yaml anchor
and aliases (tried it and it seems to work for me).

A few questions and thoughts:

- do you not need to change the default core resource limits for
FreeBSD?

- Would you find it valuable to set a few more coredump_filter bits?
Might be worth setting bits 2 and 3 (see [2])-- in addition to the
defaults (on Linux -- I don't know what the equivalent is on other
platforms).

- I found this line a bit confusing, so maybe it is worth a comment
sysinfo_script:
- export || true

- For the docker files, I think it is recommended to run "docker build"
only from within the specific build context (not in the top-level
directory), so I don't think you should need the dockerignore file.

Also, instead of putting the two docker files in the same directory,
you could put them in dedicated directories (making those directories
their build context). That way if you change one you don't end up
rebuilding the other.

- In ci/docker/linux_debian_bullseye, you can make this change:
- apt-get clean
+ apt-get clean && \
+ rm -f /var/lib/apt/lists/*

to make that layer smaller.

- Melanie

[1] https://cirrus-ci.org/guide/quick-start/
[2] https://man7.org/linux/man-pages/man5/core.5.html

Attachment Content-Type Size
use-anchors-and-aliases.patch application/octet-stream 3.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Thomas Munro 2021-11-19 22:23:01 Re: Adding CI to our tree
Previous Message Thomas Munro 2021-11-19 21:21:31 Re: [PATCH] Make ENOSPC not fatal in semaphore creation