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 appId to consul default checks #2490

Closed
wants to merge 3 commits into from

Conversation

alberthuang24
Copy link
Contributor

@alberthuang24 alberthuang24 commented Feb 1, 2023

fix #2489

Signed-off-by: AlbertHuang [email protected]

Description

Please explain the changes you've made

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #[issue number]

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

@berndverst
Copy link
Member

Is this a breaking change?

@berndverst
Copy link
Member

@alberthuang24 can you please merge the latest dapr/components-contrib@master into this PR?

@ItalyPaleAle
Copy link
Contributor

I am still not a fan of this approach. How about just changing Consul to de-register the service (if present) before registering a new one? https://pkg.go.dev/github.com/hashicorp/consul/[email protected]#Agent.ServiceDeregister

@berndverst
Copy link
Member

I am still not a fan of this approach. How about just changing Consul to de-register the service (if present) before registering a new one? https://pkg.go.dev/github.com/hashicorp/consul/[email protected]#Agent.ServiceDeregister

That does sound better.

@ItalyPaleAle ItalyPaleAle changed the title chore: Add appId to consul default checks Add appId to consul default checks Feb 24, 2023
@ItalyPaleAle
Copy link
Contributor

I've done a bunch of work on this and looks like the solution of un-registering the service wouldn't work either, because that removes all instances of the service.

I now agree with @alberthuang24 this may be the best approach to fix the issue.

@alberthuang24
Copy link
Contributor Author

@alberthuang24 can you please merge the latest dapr/components-contrib@master into this PR?

I rebase the latest master branch and will continue to resolve dapr/dapr#5868 when this PR is merged

@alberthuang24
Copy link
Contributor Author

@ItalyPaleAle Hi, Can this PR be merged?

@ItalyPaleAle
Copy link
Contributor

@alberthuang24 please see the comments here: dapr/dapr#5868

We're asking for a small change, then we can merge this and the PR in dapr/dapr

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale label Apr 26, 2023
@github-actions
Copy link

github-actions bot commented May 3, 2023

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot closed this May 3, 2023
@yaron2 yaron2 reopened this May 3, 2023
@github-actions github-actions bot removed the stale label May 3, 2023
@github-actions
Copy link

github-actions bot commented Jun 2, 2023

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale label Jun 2, 2023
@github-actions
Copy link

github-actions bot commented Jun 9, 2023

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot closed this Jun 9, 2023
@ItalyPaleAle ItalyPaleAle reopened this Jun 9, 2023
@github-actions github-actions bot removed the stale label Jun 9, 2023
@github-actions
Copy link

github-actions bot commented Jul 9, 2023

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale label Jul 9, 2023
@ItalyPaleAle
Copy link
Contributor

/keep-alive

@github-actions github-actions bot removed the stale label Jul 9, 2023
ItalyPaleAle added a commit to ItalyPaleAle/dapr-components-contrib that referenced this pull request Jul 11, 2023
Re-do of dapr#2490 to update

Co-Authored-By: AlbertHuang <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
ItalyPaleAle added a commit that referenced this pull request Jul 20, 2023
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.

Consul nameresolution resolves conflicts
4 participants