search

alan-turing-institute / data-safe-haven

https://data-safe-haven.readthedocs.io
BSD 3-Clause "New" or "Revised" License
61 stars 15 forks source link

Fix Python and R repository smoke tests #2114

Closed craddm closed 3 months ago

craddm commented 3 months ago

:white_check_mark: Checklist

:vertical_traffic_light: Depends on

This is best done in combination with an update to the nexus-allowlist container to allow installation of older versions of R packages. Current smoke tests will still fail as they require installation of older R package versions, which is not currently allowed by the Nexus server.

:arrow_heading_up: Summary

Minimally updates the PyPi allowlist, ensuring that the smoke tests can run with all required dependencies now on the list.

Updates the R smoke tests to now refer to packages that are on allowlist.

Fixes the following tests:

Does not fix the R functionality test.

:closed_umbrella: Related issues

Closes #2107

:microscope: Tests

Tested on a Tier 3 equivalent SRE deployment - noteasybeing.green.develop.turingsafehaven.ac.uk

github-actions[bot] commented 3 months ago

Coverage report

This PR does not seem to contain any modification to coverable code.

jemrobinson commented 3 months ago

@craddm : do you want to merge this as a fix for Python/PyPI only or wait until you have a fix for R/CRAN too?

craddm commented 3 months ago

@craddm : do you want to merge this as a fix for Python/PyPI only or wait until you have a fix for R/CRAN too?

Could do; smoke tests will still fail for R until nexus-allowlists is updated, and then we'll need a separate PR to update which version of that we're using. Or I can probably change the R smoke tests further so they pass now.

jemrobinson commented 3 months ago

I think it's better to keep PRs small if possible. Is this one working and ready for review?

craddm commented 3 months ago

I hadn't finished it, no, as I switched to getting the CRAN part working via nexus-allowlist, and I still need to work out why some of the smoke test are still failing.

The database smoke tests are all failing at the moment (python and R)

I suspect this is a different problem to so I'll transfer this to new issue/PR

 ✗ Postgres database (Python)
   (in test file run_all_tests.bats, line 118)
     `[ "$status" -eq 0 ]' failed
jemrobinson commented 3 months ago

I've changed the title of the PR to what I think it's doing. Can you paste a before/after of the Python tests with this change?

craddm commented 3 months ago

I've changed the title of the PR to what I think it's doing. Can you paste a before/after of the Python tests with this change?

This wasn't correct, as it also fixes an R smoke test to use packages that are actually on the allowlist

craddm commented 3 months ago

Note, this doesn't fix the R functionality test, as that requires installation of an old version of MASS that is compatible with our R version. That'd be fixed by #2116, or by changing to a different package.

JimMadge commented 3 months ago

@craddm Do you want to check if #2116 would fix this?

I think the conversation about whether than is fine to put in the release is still open. If it makes the difference here I think we should try to include it.

jemrobinson commented 3 months ago

I don't think we can release with broken smoke tests. I doubt (although I can't be completely certain) that updating the nexus-allowlists version will trigger any vulnerability that wasn't already present. If we're worried about that, let's update one of the under-test SREs and ask the penetration testers to retest the software packages server.

craddm commented 3 months ago

I also think the ramifications of this change in terms of allowing more possible downloads from CRAN are not very concerning. It's still allowing R packages that have been through the same process as all the latest R packages, and only from CRAN, so as James says I don't think it triggers any vulnerability that isn't already there.

JimMadge commented 3 months ago

I also think the ramifications of this change in terms of allowing more possible downloads from CRAN are not very concerning. It's still allowing R packages that have been through the same process as all the latest R packages, and only from CRAN, so as James says I don't think it triggers any vulnerability that isn't already there.

:+1: Moreover, I think these changes are making Nexus behave as we expected. We already decided in T2 we are comfortable with all CRAN packages, and in T3 we are not interesting in filtering versions.

jemrobinson commented 3 months ago

OK, so sounds like we should merge this PR and also test out #2116. Is that right?

craddm commented 3 months ago

I'm testing out #2116 shortly, in combination with this