-
Notifications
You must be signed in to change notification settings - Fork 36
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
multiple rancher clients #288
Conversation
LGTM, I really like this idea. I do have one question: for your hosted/tenant testing you won't need the qase reporter to report to two different qase test runs right? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we discuss this at one of the weekly design meeting?
I think I am out of context for this case, but is there a reason why it's not happening at the test level but at the client level?
So NewClient
as it is accepts a bearerToken
, so theoretically speaking, wouldn't this do the same thing?
a new config:
hostTenantTestConfig:
tenant: bearertoken
tenantHost: myhost.com
In the test:
hostClient := rancher.NewClient()
LoadAndUpdateConfig(tenantHost - for myhost.com)
tenantClient := rancher.NewClient(hostTenantTestConfig.tenant)
Couple things that I am curious about with this design:
- Currently it's per bearer token per client (as it accepts the param and if empty then checks the configuration file), but after the change, only the initial client will have this ability altough there can be multiple clients
- If the recursive configuration is picked, does
NewClient
need a change? Would a loop inRancherClients
that instantiatesNewClient
for each recursive configuration be enough?
There's also a discussion about decoupling client from the configuration file and allowing accepting the configuration object to instantiate a client, which would eliminate the example's second step too
70b47e0
to
da51c67
Compare
No, I won't. A test case checking a host/tenant issue would be reported as a single test scenario, in Qase. |
da51c67
to
f6d21e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, have you tested it Andrew?
Thanks @igomez06! Yeah I've tested and posted the results in our internal communication |
f6d21e4
to
8b75369
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There's a potential conflict with this PR: #296 Let's chat about which one goes first in the design meeting. I think it looks good definitely fixing my recursive comment. My only comment is in the config, just because it's a slice, I want to learn more about how we identify which client is what |
Purpose:
Backports: