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

[ENH] Add singularity command switcher #442

Merged
merged 9 commits into from
Jan 24, 2025
Merged

[ENH] Add singularity command switcher #442

merged 9 commits into from
Jan 24, 2025

Conversation

surchs
Copy link
Contributor

@surchs surchs commented Jan 18, 2025

Changes proposed in this pull request:

NOTE: If this pull request is to be released, the release label must be applied once the review process is done to avoid the local and remote from going out of sync as a consequence of the bump version workflow run

Checklist

This section is for the PR reviewer

  • PR has an interpretable title with a prefix ([ENH], [FIX], [REF], [TST], [CI], [MNT], [INF], [MODEL], [DOC]) (see our Contributing Guidelines for more info)
  • PR has a label for the release changelog or skip-release (to be applied by maintainers only)
  • PR links to GitHub issue with mention Closes #XXXX
  • Tests pass
  • Checks pass
  • If the PR changes the participant-level and/or the dataset-level result file, the query-tool-results files in the neurobagel_examples repo have been regenerated

For new features:

  • Tests have been added

For bug fixes:

  • There is at least one test that would fail under the original bug conditions.

Summary by Sourcery

Add a command switcher to allow users to choose between Docker and Singularity when running data retrieval commands.

New Features:

  • Added a toggle button to switch between Docker and Singularity commands for data retrieval.

Tests:

  • Added Cypress tests to verify the functionality of switching between Docker and Singularity commands.

Copy link

sourcery-ai bot commented Jan 18, 2025

Reviewer's Guide by Sourcery

This pull request introduces the ability to switch between Docker and Singularity execution environments when retrieving data. The change involves adding a toggle button group to the GetDataDialog component, which allows users to select their preferred command, and updating the copy command functionality to use the selected command.

Sequence diagram for command copy interaction

sequenceDiagram
    actor User
    participant UI as GetDataDialog
    participant System as System Clipboard

    User->>UI: Clicks command type toggle
    UI->>UI: Updates command display
    User->>UI: Clicks copy button
    UI->>System: Copies selected command
    UI->>UI: Shows copy confirmation
Loading

Class diagram for GetDataDialog component

classDiagram
    class GetDataDialog {
        +boolean open
        +function onClose()
        -string DOCKER_RUN_COMMAND
        -string SINGULARITY_RUN_COMMAND
        -string commandType
        -boolean showPopover
        -HTMLButtonElement anchorEl
        +handleCopyClick(event)
        +handlePopoverClose()
        +handleChange(event, newCommand)
        +render()
    }
    note for GetDataDialog "New commandType state and
Singularity command added"
Loading

File-Level Changes

Change Details Files
Added a toggle button group to switch between Docker and Singularity commands.
  • Added a toggle button group to the GetDataDialog component.
  • Added a state variable to track the selected command type.
  • Updated the copy command functionality to use the selected command.
  • Added a new SINGULARITY_RUN_COMMAND constant.
src/components/GetDataDialog.tsx
Added a test to verify the switching between docker and singularity commands.
  • Added a test case to verify the switching between docker and singularity commands.
cypress/component/GetDataDialog.cy.tsx

Assessment against linked issues

Issue Objective Addressed Explanation
#439 Add a command switcher between singularity and docker versions of the dataget command
#439 Ensure the singularity command works for BIC users who still use singularity
#439 Provide a user interface for switching between singularity and docker command versions

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

netlify bot commented Jan 18, 2025

Deploy Preview for neurobagel-query ready!

Name Link
🔨 Latest commit 5910039
🔍 Latest deploy log https://app.netlify.com/sites/neurobagel-query/deploys/6792fb492981eb000804761c
😎 Deploy Preview https://deploy-preview-442--neurobagel-query.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

To make tests run locally with the custom theme
Not entirely sure still why
@surchs surchs marked this pull request as ready for review January 18, 2025 20:56
@rmanaem rmanaem self-requested a review January 21, 2025 17:12
Copy link
Contributor

@rmanaem rmanaem left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @surchs!
Left a couple of comments, please take a look. Other than those 🧑‍🍳

P.S. don't forget to add a PR label

cypress/component/GetDataDialog.cy.tsx Show resolved Hide resolved
src/components/GetDataDialog.tsx Outdated Show resolved Hide resolved
src/components/GetDataDialog.tsx Outdated Show resolved Hide resolved
vite.config.ts Outdated Show resolved Hide resolved
@surchs surchs added the pr-minor Non-breaking feature or enhancement, will increment minor version (0.+1.0) label Jan 21, 2025
@surchs
Copy link
Contributor Author

surchs commented Jan 21, 2025

Should we release off of this PR as well? Thoughts @neurobagel/dev ?

@alyssadai
Copy link
Contributor

@surchs how about releasing off of #433 instead?

@surchs
Copy link
Contributor Author

surchs commented Jan 22, 2025

After discussing this at standup today, we decided to first figure out why component tests are passing locally for @rmanaem without the vite.config.ts change introduced in this PR - but fail locally for me. Once that's cleared up, we can merge the PR

@surchs surchs merged commit a089f06 into main Jan 24, 2025
14 checks passed
@surchs surchs deleted the issue439 branch January 24, 2025 02:39
@rmanaem
Copy link
Contributor

rmanaem commented Jan 24, 2025

@surchs Did u figure out why the component test suit was failing on your machine?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-minor Non-breaking feature or enhancement, will increment minor version (0.+1.0)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add singularity command for dataget
3 participants