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

add weights field for HealthService #1288

Closed
wants to merge 2 commits into from

Conversation

fengxuway
Copy link
Contributor

resolve Issue #1287

@hashicorp-cla
Copy link

hashicorp-cla commented Sep 25, 2019

CLA assistant check
All committers have signed the CLA.

@fengxuway
Copy link
Contributor Author

maybe the test code need change.

Copy link

@pierresouchay pierresouchay left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the support of weights

@phamtai97
Copy link

Can I ask if the PR has been tested and merged yet?

@fengxuway
Copy link
Contributor Author

@phamtai97 Yes. But the test code need compare expected values, so I have to change test code to add Weights field.

See here please: fengxuway@20512ea?diff=split

@eikenb eikenb added this to the 0.25.0 milestone Apr 21, 2020
@eikenb
Copy link
Contributor

eikenb commented Apr 21, 2020

I'm going to merge this for 0.25.0 but I'd like the field to be a value, not a pointer. I can make this change post merge but if you get a chance to update the PR, please do.

That is change Weights *api.AgentWeights to Weights api.AgentWeights along with the other required changes.

Thanks.

@pierresouchay
Copy link

As a side note, if the value is not set (aka not present on Consul side (ie: à Consul agent pre version 1.0.7 IIRC), the value is expected to be passing:1 and warning:1 (that's the value on Consul side when, for instance DNS retrieve it)

@eikenb
Copy link
Contributor

eikenb commented Apr 22, 2020

The conflict is due to formatting. Between that, a new test that was added and needs the Weights and changing it from a pointer to a value... Unless you speak up @fengxuway, I'm just going to pull this down, fix it up and merge from there. I'll be sure to keep attribution on the changes.

@eikenb
Copy link
Contributor

eikenb commented Apr 23, 2020

Merged via local checkout.

@eikenb eikenb closed this Apr 23, 2020
@fengxuway fengxuway deleted the fix/master/addweights branch May 18, 2022 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants