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

remove all k8s services from consul #4255

Merged
merged 13 commits into from
Aug 30, 2024

Conversation

wangxinyi7
Copy link
Member

@wangxinyi7 wangxinyi7 commented Aug 15, 2024

Changes proposed in this PR

  • Support removing the registered K8S services from consul when user runs consul sync-catalog -purge-k8s-services-from-node=k8s-sync.

How I've tested this PR

  1. Start with disabling syncCatalog
  2. Enabling syncCatalog: Confirmed from Consul UI that K8S services registered in Consul
  3. Disabling syncCatalog: registered services still in the dashboard
  4. Run consul sync-catalog -purge-k8s-services-from-node=k8s-sync
  5. Confirmed from Consul UI that registered K8S services are de-registered in Consul

How I expect reviewers to test this PR

Checklist

@wangxinyi7 wangxinyi7 self-assigned this Aug 15, 2024
@wangxinyi7 wangxinyi7 marked this pull request as draft August 15, 2024 23:22
@wangxinyi7 wangxinyi7 marked this pull request as ready for review August 16, 2024 19:18
@wangxinyi7 wangxinyi7 force-pushed the xw/NET-10571-de-register-service-command branch from 6eea669 to 0c5150d Compare August 16, 2024 19:30
@wangxinyi7 wangxinyi7 force-pushed the xw/NET-10571-de-register-service-command branch from 3c1b2df to 4c8c7ef Compare August 23, 2024 22:35
@wangxinyi7 wangxinyi7 force-pushed the xw/NET-10571-de-register-service-command branch from 4c8c7ef to a211357 Compare August 23, 2024 22:35
Copy link
Member

@nathancoleman nathancoleman left a comment

Choose a reason for hiding this comment

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

Great work! Just a few suggestions for simplifying things

control-plane/subcommand/sync-catalog/command.go Outdated Show resolved Hide resolved
control-plane/subcommand/sync-catalog/command.go Outdated Show resolved Hide resolved
control-plane/subcommand/sync-catalog/command.go Outdated Show resolved Hide resolved
control-plane/subcommand/sync-catalog/command.go Outdated Show resolved Hide resolved
control-plane/subcommand/sync-catalog/command.go Outdated Show resolved Hide resolved
control-plane/subcommand/sync-catalog/command.go Outdated Show resolved Hide resolved
control-plane/subcommand/sync-catalog/command_test.go Outdated Show resolved Hide resolved
control-plane/subcommand/sync-catalog/command.go Outdated Show resolved Hide resolved
Copy link
Member

@nathancoleman nathancoleman left a comment

Choose a reason for hiding this comment

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

Some thoughts

control-plane/subcommand/sync-catalog/command.go Outdated Show resolved Hide resolved
control-plane/subcommand/sync-catalog/command.go Outdated Show resolved Hide resolved
@wangxinyi7 wangxinyi7 enabled auto-merge (squash) August 30, 2024 18:47
Copy link
Contributor

@NiniOak NiniOak left a comment

Choose a reason for hiding this comment

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

PR looks good to me, well done 💯
I'll wait from an approval from @nathancoleman before merging, as he had some comments earlier

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