search

aisbaa / deb_packages

Provides tools to fetch debian packages for container_layer and container_image rules.
2 stars 0 forks source link

Repo overall hygenie #11

Open sluongng opened 3 years ago

sluongng commented 3 years ago

A few feedbacks I have after having gone through the code of this repo. These could be split into smaller issues if you want to address them separately

  1. In https://github.com/aisbaa/deb_packages/blob/main/deb_packages/private/deb_packages.bzl#L34 it was not obvious where and how the attributes declared to be required are used. Its a good idea to leave some in-line comment to state that these variables will be parsed and used during update process.

  2. Setup CI check for Go code: I think the go code in this repo should be put through nogo equivalent of go vet at the minimum. A better lint check would be using https://golangci-lint.run/.

  3. Make binary attributes for update rules available. Currently they comes with an underscore prefix _buildifier which suggest that it should not be modified by end-users.

  4. Don't make me download update_deb_packages_linux in https://github.com/aisbaa/deb_packages/blob/main/deb_packages/deps.bzl#L27 while I already download the http_archive of this repo. I should be able to compile the binary myself.

  5. I don't think this repo should be linux only. One should be able to download and install Deb packages onto a debian container base image all on MacOS(or window if it matter). So having all the binaries be only compatible for MacOS would be a pain.

  6. Provide documentation on update_deb_packages assumptions: currently this operates under the naive assumptions where all the dependencies are kept in WORKSPACE file. We should (a) document that assumption and (b) eventually provide a way for people to not tie themselves to WORKSPACE file and instead could use a starlark macro in a different file.

sluongng commented 3 years ago
  1. Might worth to put a big clarification that current update_deb_packages does not support dependencies resolution for a package. I.e. if traditional RUN apt install -y git would also install all the dependencies packages that git needed (perl for example), then our current deb_packages setup would only install the specified packages. This would eventually result in missing dependencies on the output container image and user would need to find our what is missing to correct it.