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

agent: enable enable_central_service_config by default #8746

Merged
merged 4 commits into from
Oct 1, 2020
Merged

agent: enable enable_central_service_config by default #8746

merged 4 commits into from
Oct 1, 2020

Conversation

rboyer
Copy link
Member

@rboyer rboyer commented Sep 24, 2020

No description provided.

@rboyer rboyer self-assigned this Sep 24, 2020
mkeeler
mkeeler reviewed Sep 24, 2020
View reviewed changes
Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

Does the agent gracefully handle the case where the servers do not have connect enabled?

@rboyer
Copy link
Member Author

rboyer commented Sep 24, 2020

(the branch is not done yet, i just figured I'd steal a little CI time to run tests for me using draft mode)

IIRC the linkage of connect being enabled and enable_central_service_config=true isn't direct. When the flag is enabled it means the ServiceManager attempts to use proxy-defaults and service-defaults during service registration for the service and it's upstreams. In the non-connect case someone just won't have any upstreams (that matter) and really the only thing that would be merged would be its own protocol value.

@rboyer
Copy link
Member Author

rboyer commented Sep 24, 2020

Hmm, TestAgent_ReloadConfigAndKeepChecksStatus fails when this flag is set because on reload a check comes back critical instead of passing

rboyer
rboyer commented Sep 24, 2020
View reviewed changes
agent/agent.go Outdated Show resolved Hide resolved
@rboyer rboyer changed the base branch from master to ensure-check-state-persists-across-reload September 24, 2020 20:50
@rboyer rboyer force-pushed the central-service-config-on-by-default branch from 4cb54b4 to 0734446 Compare September 24, 2020 20:51
Base automatically changed from ensure-check-state-persists-across-reload to master September 24, 2020 21:24
@rboyer rboyer force-pushed the central-service-config-on-by-default branch from 0734446 to 982d92d Compare September 24, 2020 21:26
@rboyer rboyer marked this pull request as ready for review September 24, 2020 22:10
@rboyer rboyer requested a review from mkeeler September 24, 2020 22:10
@rboyer rboyer added this to the 1.9 milestone Sep 29, 2020
freddygv
freddygv approved these changes Sep 30, 2020
View reviewed changes
Copy link
Contributor

@freddygv freddygv left a comment

Choose a reason for hiding this comment

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

LGTM, just missing a changelog entry

@rboyer rboyer merged commit 9801ef8 into master Oct 1, 2020
@rboyer rboyer deleted the central-service-config-on-by-default branch October 1, 2020 14:19
@rboyer rboyer mentioned this pull request Jan 20, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@freddygv freddygv freddygv approved these changes

@mkeeler mkeeler Awaiting requested review from mkeeler

Assignees

@rboyer rboyer

Labels
None yet
Projects
None yet
Milestone
1.9.0
Development

Successfully merging this pull request may close these issues.
4 participants
@rboyer @mkeeler @freddygv @hashibot-web