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.

Sign up for GitHub

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

Jump to bottom

#2396 get the latest archived flowcell #3751

Merged
merged 26 commits into from
Oct 30, 2024
Merged

#2396 get the latest archived flowcell #3751

merged 26 commits into from
Oct 30, 2024

Conversation

RasmusBurge-CG
Copy link
Contributor

@RasmusBurge-CG RasmusBurge-CG commented Sep 20, 2024 •
edited
Loading

Description

Fixed

How to prepare for test

  • Ssh to relevant server (depending on type of change)
  • Use stage: us
  • Paxa the environment: paxa
  • Install on stage (example for Hasta): 
    bash /home/proj/production/servers/resources/hasta.scilifelab.se/update-tool-stage.sh -e S_cg -t cg -b 2396-update-latest-archivedf-lowcell -a

How to test

  • Test on stage that the correct file was retrieved and correct error message was printed.
  • Test senario with only one sequence run (the normal case) and it passed.
  • tested that I got an error when faulty FC was given, that also passed.
  • Tested on FC with multiple entries on stage and the latest file was found.

Expected test outcome

  • All suggested test passed.

Review

This version is a

  • MAJOR - when you make incompatible API changes
  • MINOR - when you add functionality in a backwards compatible manner
  • PATCH - when you make backwards compatible bug fixes or documentation/instructions

Implementation Plan

  • Document in ...
  • Deploy this branch on ...
  • Inform to ...

@RasmusBurge-CG RasmusBurge-CG linked an issue Sep 20, 2024 that may be closed by this pull request
Make sure the flow cell retrieved from PDC is the latest version #2396
Closed
@RasmusBurge-CG RasmusBurge-CG changed the title 2396 update latest archivedf lowcell 2396 update latest archived flowcell Sep 20, 2024
@RasmusBurge-CG RasmusBurge-CG marked this pull request as ready for review September 23, 2024 09:54
@RasmusBurge-CG RasmusBurge-CG requested a review from a team as a code owner September 23, 2024 09:54
@RasmusBurge-CG RasmusBurge-CG changed the title 2396 update latest archived flowcell 2396 get the latest archived flowcell Sep 23, 2024
@RasmusBurge-CG RasmusBurge-CG changed the title 2396 get the latest archived flowcell #2396 get the latest archived flowcell Sep 23, 2024
@RasmusBurge-CG
Copy link
Contributor Author

Don’t hate on the branch name… 😅

islean
islean reviewed Sep 26, 2024
View reviewed changes
Copy link
Contributor

@islean islean left a comment

Choose a reason for hiding this comment

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

Looks good! There are some points I would change although the functionality is most likely correct:

  1. There seems to be some type hints missing.
  2. The Pydantic models can be written in a clearer way.
  3. We should aim to remove as many comments as possible.

cg/services/illumina/backup/backup_service.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/backup_service.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/backup_service.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/backup_service.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/utils.py Outdated Show resolved Hide resolved
cg/services/illumina/file_parsing/models.py Outdated Show resolved Hide resolved
tests/fixture_plugins/backup_fixtures/backup_fixtures.py Show resolved Hide resolved
tests/services/illumina/backup/test_backup_services.py Outdated Show resolved Hide resolved
tests/services/illumina/backup/test_backup_services.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/utils.py Outdated Show resolved Hide resolved
@Clinical-Genomics Clinical-Genomics deleted a comment from islean Oct 14, 2024
@Clinical-Genomics Clinical-Genomics deleted a comment from islean Oct 14, 2024
ChrOertlin
ChrOertlin reviewed Oct 16, 2024
View reviewed changes
cg/services/illumina/backup/utils.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/backup_service.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/backup_service.py Outdated Show resolved Hide resolved
diitaz93
diitaz93 reviewed Oct 18, 2024
View reviewed changes
Copy link
Contributor

@diitaz93 diitaz93 left a comment

Choose a reason for hiding this comment

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

Functionality-wise looks great! I left some comments to improve it

cg/services/illumina/backup/utils.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/utils.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/utils.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/validators.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/validators.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/backup_service.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/backup_service.py Outdated Show resolved Hide resolved
cg/utils/date.py Show resolved Hide resolved
tests/services/illumina/backup/test_backup_services.py Outdated Show resolved Hide resolved
tests/services/illumina/backup/test_backup_services.py Outdated Show resolved Hide resolved
RasmusBurge-CG
RasmusBurge-CG commented Oct 18, 2024
View reviewed changes
cg/utils/date.py Outdated Show resolved Hide resolved
@RasmusBurge-CG RasmusBurge-CG force-pushed the 2396-update-latest-archivedf-lowcell branch from aa35643 to 6ea50be Compare October 18, 2024 11:29
islean
islean reviewed Oct 18, 2024
View reviewed changes
Copy link
Contributor

@islean islean left a comment

Choose a reason for hiding this comment

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

Just adding some comments on casing

cg/services/illumina/backup/backup_service.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/backup_service.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/models.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/models.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/utils.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/utils.py Outdated Show resolved Hide resolved
diitaz93
diitaz93 approved these changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@diitaz93 diitaz93 left a comment

Choose a reason for hiding this comment

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

Great job @RasmusBurge-CG 🎖️

cg/services/illumina/backup/utils.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/utils.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/utils.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/utils.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/utils.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/utils.py Outdated Show resolved Hide resolved
cg/services/illumina/backup/utils.py Outdated Show resolved Hide resolved
@diitaz93
Copy link
Contributor

How to test

  • Find a flow cell that has been archived (more than once?)
  • run cg backup fetch-illumina-run -f <flow_cell_id>
  • verify that the flow cell is fetched correctly (it is the latest)

RasmusBurge-CG and others added 10 commits October 25, 2024 12:55
@RasmusBurge-CG @diitaz93
Update cg/services/illumina/backup/utils.py
b13b8f8 
Co-authored-by: Sebastian Diaz <[email protected]>
@RasmusBurge-CG @diitaz93
Update cg/services/illumina/backup/utils.py
0ad2e32 
Co-authored-by: Sebastian Diaz <[email protected]>
@RasmusBurge-CG @diitaz93
Update cg/services/illumina/backup/utils.py
4cadc5a 
Co-authored-by: Sebastian Diaz <[email protected]>
@RasmusBurge-CG @diitaz93
Update cg/services/illumina/backup/utils.py
f5c538e 
Co-authored-by: Sebastian Diaz <[email protected]>
@RasmusBurge-CG @diitaz93
Update cg/services/illumina/backup/utils.py
f3807e8 
Co-authored-by: Sebastian Diaz <[email protected]>
@RasmusBurge-CG @diitaz93
Update cg/services/illumina/backup/utils.py
411e19b 
Co-authored-by: Sebastian Diaz <[email protected]>
@RasmusBurge-CG @diitaz93
Update cg/services/illumina/backup/utils.py
120db71 
Co-authored-by: Sebastian Diaz <[email protected]>
@RasmusBurge-CG
Merge branch 'master' into 2396-update-latest-archivedf-lowcell
2ca28d1
@RasmusBurge-CG
Minor: changed one colon to equal
f84dbfe
@RasmusBurge-CG
Merge branch 'master' into 2396-update-latest-archivedf-lowcell
f63831a
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@RasmusBurge-CG RasmusBurge-CG merged commit e87ab3a into master Oct 30, 2024
9 checks passed
@RasmusBurge-CG RasmusBurge-CG deleted the 2396-update-latest-archivedf-lowcell branch October 30, 2024 14:15
@RasmusBurge-CG RasmusBurge-CG mentioned this pull request Nov 6, 2024
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrOertlin ChrOertlin ChrOertlin left review comments

@islean islean islean left review comments

@diitaz93 diitaz93 diitaz93 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make sure the flow cell retrieved from PDC is the latest version
4 participants
@RasmusBurge-CG @diitaz93 @ChrOertlin @islean