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

Consider domain in test #10

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

svalo
Copy link

@svalo svalo commented Jun 1, 2018

Addresses #9

Trying to to test an installation using test users with a domain different from the server address leads to errors

@sakchhi-kiran sakchhi-kiran self-requested a review June 18, 2018 12:38
@sakchhi-kiran
Copy link
Contributor

I have tested the changed using test domain, it is working as per the expectation.

@msmcgillis
Copy link
Contributor

msmcgillis commented Jun 25, 2018

Just some thoughts on this from my perspective.

<PROTOCOL>.domain was a quick fix to get things working when I first did this. Which was not done consistently as outlined in all the patches.

I'm also not sure about the fallback to server when domain is not available in some cases where I use IP's for servers to get through proxies this could create issues.

I don't think the global property domain is the right way to deal with this long term. My thinking was to at some point require the domain of a user be included as a column of the users.csv. This allows more flexibility with multi domain testing and associates the domain more directly with where it is needed the user.

All the code would need to be updated to support users.csv driven domains consistently then remove the domain property setting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants