Re: Adding CI to our tree

Поиск
Список
Период
Сортировка
От Melanie Plageman
Тема Re: Adding CI to our tree
Дата
Msg-id CAAKRu_bBCzU-WWKJw2TBWixnV+LU6h8jDF_XuVk+HfxPvMnmbg@mail.gmail.com
обсуждение исходный текст
Ответ на Re: Adding CI to our tree  (Andres Freund <andres@anarazel.de>)
Ответы Re: Adding CI to our tree  (Thomas Munro <thomas.munro@gmail.com>)
Re: Adding CI to our tree  (Andres Freund <andres@anarazel.de>)
Список 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

Вложения

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Thomas Munro
Дата:
Сообщение: Re: [PATCH] Make ENOSPC not fatal in semaphore creation
Следующее
От: Thomas Munro
Дата:
Сообщение: Re: Adding CI to our tree