Skip to content

feat: support remote Armadillo URLs via local_settings.csv#664

Open
timcadman wants to merge 1 commit intov6.3.6-devfrom
feat/armadillo-remote-url
Open

feat: support remote Armadillo URLs via local_settings.csv#664
timcadman wants to merge 1 commit intov6.3.6-devfrom
feat/armadillo-remote-url

Conversation

@timcadman
Copy link
Copy Markdown
Contributor

Background

Existing logic to construct armadillo url only works for localhost. To do performance testing we need to use a remote server to simualte real life conditions.

What's changed

If URL specified in local_settings is localhost it constructs url, for all other urls it uses as-is.

How to test

  • Code Review

Copy link
Copy Markdown
Member

@StuartWheater StuartWheater left a comment

Choose a reason for hiding this comment

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

I am wondering about lines 46-50, it appears to me ds.test_env$server_ip_address is just ip address, but is being assigned to a URL at line 49, so doesn't specify protocol or port of URL.

Have I missed something?

I think it was intended for remote host name to be placed in "local_settings.csv" if needs to be specified.

@timcadman
Copy link
Copy Markdown
Contributor Author

Maybe I've missed something! The issue I had was that it works fine if you specify localhost in the local_settings.csv, but if I want to specify e.g armadillo-demo.molgenis.net I don't want the port pasted on the end. So tried to construct so the port is only added for localhost - but perhaps there is a better way?

@StuartWheater
Copy link
Copy Markdown
Member

OK, but what if a port needs to be added to a hostname other than localhost. For example, if I the test server is http://datashield-big.home:8080/

What make is more complicated is that there is one test that needs a hostname not an url.

(PS: Not sure why CSV was chooses, JSON would be more flexible)

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants