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

Add snapshots option to cli pull command #732

Merged
merged 4 commits into from
Jul 17, 2024

Conversation

midigofrank
Copy link
Contributor

@midigofrank midigofrank commented Jul 16, 2024

Short Description

Adds --snapshots option to cli pull command

Related issue

Fixes #729

QA Notes

  • openfnx pull {projectId} should work as it was before
  • adding --snapshots option appends the snapshots to the query params. i.e ?snapshots[]=1&snapshots[]=2

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have added unit tests
  • Changesets have been added (if there are production code changes)

@midigofrank midigofrank self-assigned this Jul 16, 2024
@midigofrank midigofrank marked this pull request as draft July 16, 2024 13:18
@midigofrank
Copy link
Contributor Author

@josephjclark I'm unable to find the tests for cli pull command and so I'm not even sure how to start writing tests for my changes. Do you have some pointers?

@josephjclark
Copy link
Collaborator

@midigofrank Ack, you're right. Our testing across deploy and pull has always been extremely poor and it's never been important enough to fix.

We'll test any new CLI commands manually as part of QA on this branch. So no CLI tests needed.

In packages/deploy, could you write some tests against getLightningUrl ? They can go in index.test.ts

@midigofrank midigofrank force-pushed the add-snapshots-option-cli-pull branch from 39d31fa to a1cebb2 Compare July 17, 2024 06:28
@midigofrank midigofrank marked this pull request as ready for review July 17, 2024 06:37
@josephjclark
Copy link
Collaborator

josephjclark commented Jul 17, 2024

@midigofrank aside from the typo this looks fine, but I'm curious about the URL

This is a strange format to me:

?snapshots[]=1&snapshots[]=2

What's up with the square brackets?

I suppose this is a lightning design thing, but I wo?snapshots[]=1&snapshots[]=2

I would expect either of the following formats:

?snapshot=1&snapshot=2
?snapshots=1,2

@midigofrank
Copy link
Contributor Author

Hey @josephjclark, the square braces is a convention for handling array data sent through forms or just get requests.

When phoenix sees that kind of data, it will correctly parse it as an array. This same approach is used to submit nested data. You can see this in action in the workflow editor form.

Here's the input for editing the first job's name:

<input class="" id="workflow_jobs_0_name" name="workflow[jobs][0][name]" type="text" value="Transform data to FHIR standard">

This gets parsed as: {workflow: {jobs: {0: {name: "Transform data to FHIR standard"}}}}

@josephjclark
Copy link
Collaborator

josephjclark commented Jul 17, 2024

Cool, I've never seen that before.

I'll approve once the typo is fixed. I haven't tested this at all. Do you want someone to run QA or shall we just release it? Looks like quite a safe change

@midigofrank
Copy link
Contributor Author

I see no harm at all, lets just release it.

@josephjclark josephjclark merged commit 9928992 into main Jul 17, 2024
1 of 2 checks passed
@josephjclark josephjclark deleted the add-snapshots-option-cli-pull branch July 17, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

CLI: add an option to pull that targets given snapshots
2 participants