nwiltsie
commented
10 months ago Odd, I thought I disabled the cross-file similarity checker... https://github.com/uclahs-cds/docker-CICD-base/pull/68
Closed nwiltsie closed 10 months ago
nwiltsie
commented
10 months ago Odd, I thought I disabled the cross-file similarity checker... https://github.com/uclahs-cds/docker-CICD-base/pull/68
nwiltsie
commented
10 months ago Okay, I had to pull the common selector-based code into a common method to make the linter happy (apparently that comment broken on different lines made all the difference to it), but things still work as expected.
/hot/software/pipeline/pipeline-recalibrate-BAM/Nextflow/development/unreleased/main/log-nftest-20240112T005925Z.log
yashpatel6
commented
10 months ago Changes look good! I think the tests will need to be updated to reflect the selector-based code being moved around
nwiltsie
commented
10 months ago Ah yeah I didn't run the tests for myself (there should be an action for that...) but I'll do so this morning.
nwiltsie
commented
10 months ago Okay, the tests now run correctly:
$ git rev-parse HEAD
aca1840371ee339fea01af8703768265dd5bf76b
$ pytest
============================= test session starts ==============================
platform linux -- Python 3.10.7, pytest-7.4.4, pluggy-1.3.0
rootdir: /hot/code/nwiltsie/tools/tool-NFTest
collected 17 items
test/unit/test_NFTestAssert.py ......... [ 52%]
test/unit/test_NFTestCase.py .. [ 64%]
test/unit/test_NFTestEnv.py .. [ 76%]
test/unit/test_NFTestRunner.py . [ 82%]
test/unit/test_common.py ... [100%]
============================== 17 passed in 0.70s ==============================
Description
This PR switches both instances of
subprocess.Popen(launching Nextflow and launching the custom assert script) to useshell=False.It's generally recommended to use
shell=Falseto avoid any potential shell injections.One subtle change associated with this is that we have to set the
NXF_WORKenvironment variable via theenvargument ofsubprocess.Popen, rather than embedding it in the command string.envis the complete set of environment variables so we have to merge the current process's environment with our changes ({**os.environ, **envmod}).I tested this by adding a custom comparison script to pipeline-recalibrate-BAM's NFTest suite:
The NFTest log file (
/hot/software/pipeline/pipeline-recalibrate-BAM/Nextflow/development/unreleased/main/log-nftest-20240111T231156Z.log) shows that both the Nextflow invocation...... and the comparison invocations...
... are well-formatted and function appropriately (
compare.pyjust prints the inputs and exits with0).Checklist
[x] This PR does NOT contain Protected Health Information (PHI). A repo may need to be deleted if such data is uploaded.
Disclosing PHI is a major problem[^1] - Even a small leak can be costly[^2].
[x] This PR does NOT contain germline genetic data[^3], RNA-Seq, DNA methylation, microbiome or other molecular data[^4].
[^1]: UCLA Health reaches $7.5m settlement over 2015 breach of 4.5m patient records [^2]: The average healthcare data breach costs $2.2 million, despite the majority of breaches releasing fewer than 500 records. [^3]: Genetic information is considered PHI. Forensic assays can identify patients with as few as 21 SNPs [^4]: RNA-Seq, DNA methylation, microbiome, or other molecular data can be used to predict genotypes (PHI) and reveal a patient's identity.
.png, .jpeg),.pdf,.RData,.xlsx,.doc,.ppt, or other output files.To automatically exclude such files using a .gitignore file, see here for example.
[x] I have read the code review guidelines and the code review best practice on GitHub check-list.
[x] I have set up or verified the
mainbranch protection rule following the github standards before opening this pull request.[x] The name of the branch is meaningful and well formatted following the standards, using [AD_username (or 5 letters of AD if AD is too long)]-[brief_description_of_branch].
[x] I have added the major changes included in this pull request to the
CHANGELOG.mdunder the next release version or unreleased, and updated the date.