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

Remove "-d" short flag from "delivery-type" and "days-ago" options #3238

Closed
wants to merge 14 commits into from

Conversation

eliottBo
Copy link
Contributor

Description

Fixing issue #3234
Remove "-d" short flag from "delivery-type" and "days-ago" option to avoid conflict with "-d" from dry-run option.

Changed

  • Remove "-d" short flag from "delivery-type" option in /Users/eliottbosshard/cg/cg/cli/deliver/base.py and "days-ago" option in /Users/eliottbosshard/cg/cg/cli/upload/mutacc.py

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 [THIS-BRANCH-NAME] -a

How to test

  • Do ...

Expected test outcome

  • Check that ...
  • 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 ...

@eliottBo eliottBo requested a review from a team as a code owner May 16, 2024 12:59
@eliottBo
Copy link
Contributor Author

eliottBo commented May 16, 2024

The following documentation will need to be changed as well in Atlas:

  • /atlas/production/data_analysis/workflows/microsalt/
  • /atlas/docs/production/data_analysis/supporting_information/supportsystem_usage.md
  • /atlas/docs/production/data_analysis/supporting_procedures/delivery.md
  • /atlas/docs/production/data_analysis/workflows/balsamic.md
  • /atlas/docs/production/data_analysis/workflows/microsalt.md
  • /atlas/docs/production/data_analysis/workflows/raw_data_delivery.md
  • /atlas/docs/production/data_analysis/workflows/sars-cov-2.md

Copy link
Contributor

@ivadym ivadym left a comment

Choose a reason for hiding this comment

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

Nice 👍

Great work identifying the affected atlas documents 💯

Copy link
Contributor

@henrikstranneheim henrikstranneheim left a comment

Choose a reason for hiding this comment

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

👍

@diitaz93
Copy link
Contributor

I spoke with @Karl-Svard and production would prefer removing the -d of dry-run instead, as they use the -d of delivery on a daily basis almost

@henrikstranneheim
Copy link
Contributor

Ok, sounds good! Usability is key here I think.

@eliottBo
Copy link
Contributor Author

Removing the -d flag would also mean to change the tests in place.
Isn't @ivadym's suggestion to change the delivery-type flag to "-dt" and the days-ago flag to "-da" the easiest way? In this way they are still short and easy to use in production and all we have is to change the documentation.
(Just a suggestion)

@ivadym
Copy link
Contributor

ivadym commented May 17, 2024

Removing the -d flag would also mean to change the tests in place. Isn't @ivadym's suggestion to change the delivery-type flag to "-dt" and the days-ago flag to "-da" the easiest way? In this way they are still short and easy to use in production and all we have is to change the documentation. (Just a suggestion)

Since we'll be deprecating the data delivery flag, I would say we remove the -d from the dry run and avoid messing with prod bioinfo. I also think it makes sense to be more explicit when calling the --dry-run, and on top of that the short version is usually -n, not -d.

Copy link

sonarcloud bot commented May 17, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@eliottBo eliottBo linked an issue May 17, 2024 that may be closed by this pull request
Comment on lines -304 to -324
class PacbioConfig(BaseModel):
data_dir: str
systemd_trigger_dir: str


class OxfordNanoporeConfig(BaseModel):
data_dir: str
systemd_trigger_dir: str


class IlluminaConfig(BaseModel):
flow_cell_runs_dir: str
demultiplexed_runs_dir: str


class RunInstruments(BaseModel):
pacbio: PacbioConfig
nanopore: OxfordNanoporeConfig
illumina: IlluminaConfig


Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to check your branch. This undoes some changes introduced with other prs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I had a problem with git, I created another branch (Remove dryrun flag #3244) to replace this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to close this one to avoid confusion :)

@diitaz93
Copy link
Contributor

Closing, replaced by #3244

@diitaz93 diitaz93 closed this May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants