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

add beacon import and search tools #6028

Merged
merged 21 commits into from
Jul 22, 2024
Merged

add beacon import and search tools #6028

merged 21 commits into from
Jul 22, 2024

Conversation

khaled196
Copy link
Contributor

@khaled196 khaled196 commented May 24, 2024 •
edited
Loading

FOR CONTRIBUTOR:

  • I have read the CONTRIBUTING.md document and this tool is appropriate for the tools-iuc repo.
  • License permits unrestricted use (educational + commercial)
  • This PR adds a new tool or tool collection
  • This PR updates an existing tool or tool collection
  • This PR does something else (explain below)

@khaled196
Copy link
Contributor Author

@bgruening

HI Bjorn as suspected the tools failed the testing. Could you please help me with that?

The tools use IP addresses for beacon queries and forward the results into a file.

@khaled196 khaled196 marked this pull request as draft May 24, 2024 11:27
@bgruening
Copy link
Member

The errors are complaint about a missing port on localhost. But you specify an IP with 20.x.x.x in the tests. Is this a public server? Any idea why it's not used?

@khaled196
Copy link
Contributor Author

The errors are complaint about a missing port on localhost. But you specify an IP with 20.x.x.x in the tests. Is this a public server? Any idea why it's not used?

I found the reason and it was a mistake in adding the parameter db_host. I hope it will work now

@khaled196 khaled196 marked this pull request as ready for review May 24, 2024 14:00
bgruening
bgruening reviewed May 27, 2024
View reviewed changes
tools/beacon2-import/macros.xml Outdated
<param argument="--advance-connection" type="boolean" checked="false" truevalue="--advance-connection" falsevalue="" label="ADVANCE CONNECTION" help="Connect to beacon database with authentication" />
<param argument="--db-auth-source" optional="true" type="text" label="DATABASE AUTHENTICATON SOURCE" value="admin" help="Auth source for the beacon database" />
<param argument="--db-user" optional="true" type="text" label="DATABASE USER" value="" help="Login user-name for the beacon database" />
<param argument="--db-password" optional="true" type="text" label="DATABASE PASSWORD" value="" help="Login password for the beacon database" />
Copy link
Member

Choose a reason for hiding this comment

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

We should not and never put passwords in as parameters. Instead, I would recommend putting those credentials into the user-preference and then accessing those credentials via a tool during runtime. This is working currently for other tools as well.

Related to this we have some ideas to make it easier to "annotate" tools to request credentials. @khaled196 @poterlowicz-lab do you have time and joy to hack on this? It would involve Galaxy core code both backend and a bit frontend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @bgruening ,

Thank you for letting me know this. Sure. I am happy to learn how to do this. However, I am unfamiliar with Galaxy's back and front-end scripts. Please also recommend documentation to help me understand what I should do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should not and never put passwords in as parameters. Instead, I would recommend putting those credentials into the user-preference and then accessing those credentials via a tool during runtime. This is working currently for other tools as well.

Related to this we have some ideas to make it easier to "annotate" tools to request credentials. @khaled196 @poterlowicz-lab do you have time and joy to hack on this? It would involve Galaxy core code both backend and a bit frontend.

Hi @bgruening

I wanted to follow up with you on my last message about the credentials integration for the tool I'm wrapping on Galaxy. I'm eager to start on this but I would appreciate some guidance or documentation on the Galaxy core code, both backend and frontend.

If you have any resources or can provide a bit of direction on how to proceed, it would be very helpful.

Thank you

Copy link
Member

Choose a reason for hiding this comment

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

Upps, sorry have not seen your response.

The issue is here: galaxyproject/galaxy#17511

You are free to discuss details there or make a different suggestion.
The high level idea is:

  • annotate in tools eine art section
  • add this to the XSD schema
  • make Galaxy parse it and put this into a dedicated namespace in the user-preference store
  • put a UI into the user preference that lets users add/remove credentials
  • (optional) If a tool is started but no credentials are given the tool should not start and the user should be pointed to the enter credentials

@khaled196 khaled196 mentioned this pull request May 30, 2024
Open
@khaled196
Copy link
Contributor Author

Hi @bgruening

I applied what I understood from the documentation and examples.

is this what you mean by annotate in tools eine art section

and modify this to add the user and the other info in the user preference. https://github.com/galaxyproject/galaxy/blob/bad3f8a3ba1f340ab0b791911b43bdeb2b25bd60/client/src/nls/es/locale.js#L654

@khaled196 khaled196 mentioned this pull request Jul 15, 2024
Closed
4 tasks
@bgruening
Copy link
Member

Job in error state.. tool_id: beacon2_analyses, exit_code: 2, stderr: /tmp/tmpelzsvzus/job_working_directory/000/1/tool_script.sh: line 23: jq: command not found
/tmp/tmpelzsvzus/job_working_directory/000/1/tool_script.sh: line 23: jq: command not found
/tmp/tmpelzsvzus/job_working_directory/000/1/tool_script.sh: line 23: jq: command not found
usage: beacon2-search analyses [-h] [-H DATABASE_HOST] [-P DATABASE_PORT] [-a]
[-A ADMIN] [-U DATABASE_USER]
[-W DATABASE_PASSWORD] [-d DATABASE]
[-c COLLECTION] [-al ALIGNER]
[-ad ANALYSISDATE] [-bi BIOSAMPLEID]
[-id IDENTIFICATION] [-ii INDIVIDUALID]
[-pn PIPELINENAME] [-pr PIPELINEREF]
[-ri RUNID] [-vc VARIANTCALLER]
beacon2-search analyses: error: argument -A/--db-auth-source: expected one argument

@khaled196
Copy link
Contributor Author

@bgruening

I made the same mistake as before with the config_file. I removed it by including it in the code. This should work now.

@khaled196
Copy link
Contributor Author

@bgruening

I have updated the tool to work with a config file to save credentials

@bgruening
Copy link
Member

Much cleaner, thanks!

@bgruening bgruening merged commit f4151aa into galaxyproject:main Jul 22, 2024
14 checks passed
@khaled196 khaled196 deleted the import branch July 22, 2024 12:25
nilchia pushed a commit to pavanvidem/tools-iuc that referenced this pull request Aug 24, 2024
@khaled196 @nilchia
add beacon import and search tools (galaxyproject#6028)
c5cb51f 
* add new tools

* add beacon import and search

* add the beacon import and beacon search

* update tools help and tests

* update tool version

* fix .shed text

* fix .shed text

* fix help part in gene

* fix .shed text

* fix linting

* fix ip

* remove password from parmetars

* fix linting

* fix linting

* add more query options and update the script to the latest version

* change the output file format and onother linting errors

* fix spilling

* remove config_file

* add configFile
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bgruening bgruening bgruening 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.
2 participants
@khaled196 @bgruening