Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#69 add apt proxy repo support #70

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Jbaudon
Copy link
Contributor

@Jbaudon Jbaudon commented Nov 13, 2024

Description

Fixes #69

This PR simplifies the installation of Debian packages required by R packages by configuring a Nexus APT proxy repository and updating the related allowlist.
Additionally, it introduces the option to pass certain parameters as environment variables, enabling users to modify the remote source of proxy repositories and specify the APT distribution version. This also allows for the use of private repositories (PyPI, CRAN, or APT) or distributions other than Debian (Ubuntu for instance).

Caveat

I did not implement integration tests because doing so would require modifying the base image to a Debian-based one in integration_tests/Dockerfile. I’m not comfortable making such changes. If you have any suggestions regarding this, I would appreciate your input.

@Jbaudon Jbaudon marked this pull request as ready for review November 19, 2024 13:52
@JimMadge JimMadge requested review from JimMadge and craddm November 19, 2024 14:41
Copy link
Member

@JimMadge JimMadge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks so much 🎉.

I will look to test this soon, but I can't make a guarantee right now.

Regarding integration tests, I think the Bats container image is build on Alpine Linux. There is an apt package in their repos. So it might be possible to add tests after running apk add apt in the container build.

nexus_allowlist/settings.py Outdated Show resolved Hide resolved
nexus_allowlist/actions.py Outdated Show resolved Hide resolved
nexus_allowlist/actions.py Outdated Show resolved Hide resolved
@JimMadge JimMadge self-assigned this Dec 6, 2024
Comment on lines 1 to 9
import os

PYPI_REMOTE_URL = os.getenv("PYPI_REMOTE_URL", "https://pypi.org/")
CRAN_REMOTE_URL = os.getenv("CRAN_REMOTE_URL", "https://cran.r-project.org/")
APT_REMOTE_URL = os.getenv("APT_REMOTE_URL", "http://deb.debian.org/debian")
APT_DISTRO = os.getenv("APT_DISTRO", "bookworm")
ALLOWED_ARCHIVES = os.getenv(
"APT_ALLOWED_ARCHIVES", "main,contrib,non-free-firmware,non-free"
).split(",")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would prefer these to be passed to the program as arguments, to be consistent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also feels risky because we skip any validation and type checking.

@JimMadge
Copy link
Member

JimMadge commented Dec 6, 2024

Not certain why mypy is failing. There are issues saying the "no cache dir" message is misleading, but it passes locally for me.

Comment on lines 3 to 4
PYPI_REMOTE_URL = os.getenv("PYPI_REMOTE_URL", "https://pypi.org/")
CRAN_REMOTE_URL = os.getenv("CRAN_REMOTE_URL", "https://cran.r-project.org/")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd want to do these in another PR.

Comment on lines +129 to +130
if repo_type == RepositoryType.APT:
payload["apt"] = {"distribution": APT_DISTRO, "flat": False}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks problematic. I think what we probably want to do is pass an object with the repositories actual parameters.

@JimMadge
Copy link
Member

JimMadge commented Dec 6, 2024

I think this has highlighted some areas where the structure of the code (with hard-coded parameters and assumptions) has made it too inflexible.
I think it might be best to do this as part of a restructure rather than hacking in workarounds to allow configuring the apt repository (#71).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

Extend Nexus Allowlist to Support Debian Packages Required for R Package Installation
2 participants