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

Changes to CLI to support Axios Proxy, Large FHIR List Downloads, and Downloads of Layouts #80

Merged
merged 37 commits into from
May 29, 2019
Merged

Changes to CLI to support Axios Proxy, Large FHIR List Downloads, and Downloads of Layouts #80

merged 37 commits into from
May 29, 2019

Conversation

taylordeatri
Copy link
Contributor

Changes to support configuring Proxy

Changed FHIR list to support option to output in a CSV, TSV, etc. format using a json file that maps JmesPaths to column names for building a CSV from the deep json structure.

  • Changed to print results as they are received by default (was by an option before.)
  • Also added a global --jsonLine print option to support printing the json one resource per line (i.e., without pretty print).

Added apps.js to support calling application APIs

  • Used to get Configured Layouts for a project.

Also updated some dependencies which resolved some issues I encountered using Axios Proxy support.

@taylordeatri
Added support for proxy and added support for apps url for downloadin…
0ae387c 
…g insights and patient layouts.
@taylordeatri
Added unit test for get configuration_layouts command.
9b21842
@taylordeatri
Updated list to show each bundle as it comes in.
b1f2fbd
@taylordeatri
Merged Lifeomic latest.
6a88d6f
@taylordeatri
Uncommented out lines in build.sh
51dccf3
@taylordeatri
Fixed issues with listing large amounts of data.?
3b27dc4
@taylordeatri
Added proxy to api.js and apps.js
3d20c96
@taylordeatri
Updated package-lock.json
1703616
@taylordeatri
Merge branch 'master' into master
00aa8a4
@taylordeatri
Updated code based on failed build.
6dc9fae
@taylordeatri
Added missing config for httpsProxy.
104c280
@taylordeatri
Removed upload/download methods in apps.js and renamed configuration_…
70a954e 
…layouts to layouts.
@taylordeatri
Fixed lint errors.
805b7a6
@taylordeatri
Renamed and fixed layouts-get.test.js
342d0ad
@taylordeatri
Adjusted config.js and renamed test class.
8c7ee5c
@taylordeatri
Fixed missing brace.
93eba8a
@taylordeatri
Fixed lint issue.
69eb5a1
@taylordeatri
PHD-417 Updated list to dump output per bundle rather than collect th…
3a96497 
…em all.
@taylordeatri
PHD-417 Added --jsonLine to make line per resource output and --csv t…
0dd0778 
…o support a json to CSV conversion via a specification with the mappings of fhir jmesPath to column mapping.
@taylordeatri
PHD-417 restored build.sh
f82c2c4
@taylordeatri
Merge pull request lifeomic#1 in PHI/phi_cli from feature/PHD-417-jme…
09a0b25 
…spath-csv to master

* commit 'f82c2c4e5c0f71ea3d1c2e87d40eae74bff2477b':
  PHD-417 restored build.sh
  PHD-417 Added --jsonLine to make line per resource output and --csv to support a json to CSV conversion via a specification with the mappings of fhir jmesPath to column mapping.
  PHD-417 Updated list to dump output per bundle rather than collect them all.
@taylordeatri
Merged LO master to update.
dfe3b48
@taylordeatri
Added changes from the Hackathon - including removing the logResults …
ac9cb99 
…parameter.
@taylordeatri
Added sample csv/tsv format files.
f3469a7
@taylordeatri
Fixed some lint errors.
cdcc699
@taylordeatri
Removed defaults from the config.js.
701c3ad
mschroering
mschroering reviewed May 15, 2019
View reviewed changes
csv_medadmin.json Outdated Show resolved Hide resolved
@hemp
Copy link
Member

hemp commented May 17, 2019

I think the package-lock.json file can be removed to avoid confusion on which package manager is in use (yarn).

mschroering
mschroering reviewed May 28, 2019
View reviewed changes
test/unit/commands/fhir-test.js Outdated
@@ -61,6 +61,7 @@ test.serial.cb('The "fhir" command should list fhir resources', t => {

yargs.command(list)
.parse('list Patient --project projectId');
t.end();
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 this will end the test before the verification completes in the callback.

Copy link
Contributor

Choose a reason for hiding this comment

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

You may need to mock out there proxy module here like you did for api.test.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks - I'll re-test it with the mock.

@mschroering
Copy link
Contributor

I think the package-lock.json file can be removed to avoid confusion on which package manager is in use (yarn).

This file should still be removed as the project uses a yarn lock file.

@taylordeatri
Copy link
Contributor Author

I tried axios 1.18.0 and get this error when hitting the outbound proxy:

./lo fhir list Patient --limit 1
Error: write EPROTO 140631739561856:error:140770FC:SSL routines:SSL23_GET_SERVER_HELLO:unknown protocol:../deps/openssl/openssl/ssl/s23_clnt.c:827:

This appears to be related to this issue which is fixed in 1.19.0:
axios/axios#925

So to enable the proxy it appears we need the 1.19.0-beta release.

@anthonyroach
Copy link
Contributor

@taylordeatri I also tried using 1.18.0 with a proxy and got the same error. Looks like using the beta is the only option for now.

@mschroering
Copy link
Contributor

@taylordeatri I also tried using 1.18.0 with a proxy and got the same error. Looks like using the beta is the only option for now.

@anthonyroach you were able to get this to work with the axios beta, right?

https_proxy=https://45.76.76.19:3128 lo projects list

@taylordeatri can you use the environment variable approach? Then we would not need to add the proxy configuration to the setup.

@anthonyroach
Copy link
Contributor

@k

@anthonyroach you were able to get this to work with the axios beta, right?

https_proxy=https://45.76.76.19:3128 lo projects list

Yes I was able to get that working with the axios beta.

@anthonyroach anthonyroach reopened this May 28, 2019
@anthonyroach
Copy link
Contributor

Oops, didn't mean to close this PR.

@taylordeatri
Copy link
Contributor Author

I'm closing this - tested and confirmed that with the 1.19.0-beta.1 version of axios the property https_proxy DOES work. I'm removing the Proxy configuration code in the changes and will re-submit a new PR.
Thank you Anthony and Mark!

@anthonyroach anthonyroach reopened this May 28, 2019
@mschroering mschroering merged commit ea21881 into lifeomic:master May 29, 2019
taylordeatri added a commit to taylordeatri/cli that referenced this pull request Apr 21, 2020
@taylordeatri
Merge remote-tracking branch 'lo_cli_origin/master' - Merging from Li…
4b9595a 
…feomic master post PR lifeomic#80 merge.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mschroering mschroering mschroering 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.
4 participants
@taylordeatri @hemp @mschroering @anthonyroach