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

feat(raredisease): Add raredisease to cg workflow #2592

Merged
merged 10 commits into from
Oct 31, 2023
Merged

feat(raredisease): Add raredisease to cg workflow #2592

merged 10 commits into from
Oct 31, 2023

Conversation

peterpru
Copy link
Member

@peterpru peterpru commented Oct 16, 2023 •
edited
Loading

Description

This PR adds the cli option raredisease to cg workflow, without added options.

Added

  • cli option: cg workflow raredisease

Changed

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 add-raredisease-to-cg-workflow -a

How to test

  • Do cg workflow --help

Expected test outcome

  • Check that installing on stage shows that the alembic revision is installed correctly.

  • image

  • cg workflow shows the new option:

  • image

  • When completing the tests, pull master and show that the alembic downgrade completed successfully.

  • image

  • Take a screenshot and attach or copy/paste the output.

Review

  • Tests executed by
  • "Merge and deploy" approved by
    Thanks for filling in who performed the code review and the test!

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 ...

@peterpru peterpru self-assigned this Oct 16, 2023
@peterpru peterpru added raredisease Issues related to nextflow raredisease pipeline cg labels Oct 16, 2023
@henrikstranneheim
Copy link
Contributor

@peterpru It would be great if you keep the scope small say one PR per command.

@peterpru peterpru changed the title Add raredisease to cg workflow feat(raredisease): Add raredisease to cg workflow Oct 23, 2023
@peterpru peterpru marked this pull request as ready for review October 23, 2023 11:28
@peterpru peterpru requested a review from a team as a code owner October 23, 2023 11:28
@peterpru
Copy link
Member Author

peterpru commented Oct 23, 2023 •
edited
Loading

Consulted with Vincent; recommendation is to ignore the SonarCloud duplication % for this PR. This is a short script used in version control, and outside the active "cg production code".

@karlnyr karlnyr requested review from fevac and islean October 23, 2023 11:48
islean
islean approved these changes Oct 23, 2023
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.

I think it looks good! For tests, it would be good to deploy to hasta and cg-vm1 and make sure that the new option shows up in the web interface. Also make sure that deployment of e.g. master executes the downgrade as you want it to.

alembic/versions/dbe513da5cb9_add_raredisease_to_analysis_options.py Outdated Show resolved Hide resolved
alembic/versions/dbe513da5cb9_add_raredisease_to_analysis_options.py Outdated Show resolved Hide resolved
alembic/versions/dbe513da5cb9_add_raredisease_to_analysis_options.py Outdated Show resolved Hide resolved
alembic/versions/dbe513da5cb9_add_raredisease_to_analysis_options.py Outdated Show resolved Hide resolved
cg/cli/workflow/raredisease/base.py Show resolved Hide resolved
@islean
Copy link
Contributor

islean commented Oct 23, 2023

Just to be clear - I think this will probably work, but you can probably clean up the alembic script a little bit.

fevac
fevac requested changes Oct 23, 2023
View reviewed changes
Copy link
Contributor

@fevac fevac left a comment

Choose a reason for hiding this comment

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

Nice!! 🌟 🌟 so exciting seeing this happening! I think it's important to avoid having duplicated code, so I added a suggestion to merge all similar NF tests. See below

alembic/versions/dbe513da5cb9_add_raredisease_to_analysis_options.py Outdated Show resolved Hide resolved
tests/cli/workflow/raredisease/test_cli_raredisease_compound_commands.py Show resolved Hide resolved
fevac
fevac approved these changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@fevac fevac left a comment

Choose a reason for hiding this comment

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

🚀 you can link the issue/pr to refactor the tests if you have already created it

@peterpru peterpru mentioned this pull request Oct 30, 2023
Closed
@peterpru
Copy link
Member Author

🚀 you can link the issue/pr to refactor the tests if you have already created it

Here is the created issue for the refactoring: #2643

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@peterpru
Copy link
Member Author

peterpru commented Oct 30, 2023 •
edited
Loading

Successfully installs on hasta stage: image

Also succesfully downgrades:
image

@clingen-sthlm clingen-sthlm temporarily deployed to stage October 31, 2023 07:26 Inactive
@peterpru
Copy link
Member Author

peterpru commented Oct 31, 2023 •
edited
Loading

Edit: had green light in actions, looked okay in terminal and deploying to cg-vm1 using Actions, and testing on stage after, after merging the error above popped up. Reverting the merge in #2646

@peterpru peterpru merged commit d7fdd9f into master Oct 31, 2023
9 checks passed
@peterpru peterpru deleted the add-raredisease-to-cg-workflow branch October 31, 2023 07:33
peterpru added a commit that referenced this pull request Oct 31, 2023
@peterpru
Revert "feat(raredisease): Add raredisease to cg workflow (#2592)"
fd6c798 
This reverts commit d7fdd9f.
@peterpru peterpru restored the add-raredisease-to-cg-workflow branch October 31, 2023 07:34
@peterpru peterpru mentioned this pull request Oct 31, 2023
Merged
peterpru added a commit that referenced this pull request Oct 31, 2023
@peterpru
Revert "feat(raredisease): Add raredisease to cg workflow (#2592)" (#…
54f95be 
…2646)

This reverts commit d7fdd9f.
@peterpru peterpru deleted the add-raredisease-to-cg-workflow branch October 31, 2023 08:11
@peterpru peterpru mentioned this pull request Oct 31, 2023
Merged
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vince-janv Vince-janv Vince-janv left review comments

@islean islean islean approved these changes

@fevac fevac fevac approved these changes

Assignees

@peterpru peterpru

Labels
cg raredisease Issues related to nextflow raredisease pipeline
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@peterpru @henrikstranneheim @islean @fevac @Vince-janv @clingen-sthlm