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 support for LocalConnectTimeoutMs and LocalRequestTimeoutMs on service-defaults CRD #1642

Closed

Conversation

erdanzhang
Copy link
Contributor

Changes proposed in this PR:

Add maxInboundConnections to service-defaults CRD
fix #1641
Checklist:

  • Tests added

Reference of similar PR before adding entries to serviceDefault:
#1437

@hashicorp-cla
Copy link

hashicorp-cla commented Oct 21, 2022

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

Have you signed the CLA already but the status is still pending? Recheck it.

@david-yu
Copy link
Contributor

Thank you @erdanzhang. Could you also sign the CLA as well? We will take a look.

@erdanzhang
Copy link
Contributor Author

@david-yu Thanks for the quick reply.
image
I guess I already signed, but maybe because I use the a different email address other than the github hub account one, the system seems to fail to recognize it.
Is there any way to fix that? Someway I could edit my email in hashicorp page, or let me resign it from start?
Thanks!!!

Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

Hey @erdanzhang !! Thank you so much for your contribution. Have a comment about how the CRD in the templates folder has been generated.

Comment on lines 10 to 11
app: consul
chart: consul-helm
Copy link
Contributor

Choose a reason for hiding this comment

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

The changes to this file should be autogenerated by running make ctrl-generate ctrl-manifests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thisisnotashwin Thanks for the comments!! Auto-gen code is cool
After I run make ctrl-generate ctrl-manifests, there are few other files got changed as well

	modified:   charts/consul/templates/crd-peeringacceptors.yaml
	modified:   charts/consul/templates/crd-peeringdialers.yaml
	modified:   charts/consul/templates/crd-servicedefaults.yaml
	modified:   control-plane/api/v1alpha1/zz_generated.deepcopy.go
	modified:   control-plane/config/crd/bases/consul.hashicorp.com_servicedefaults.yaml

Could you help double check if the changes are intended?

@david-yu
Copy link
Contributor

Hi @erdanzhang you would either sign with the email associated with your GitHub account or amend the author email on the git commits associated with that PR to match the address from their cla submission. These are the only ways to get the CLA to pass from chatting with the CLA team.

@erdanzhang
Copy link
Contributor Author

@david-yu @thisisnotashwin Seems the CLA check still failed after I amended the git author email to the one used to sign the agreement.
I end up teleporting this PR to a new PR:
#1647
Sorry about the friction, let's move our discussion to the new PR.

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.

crd-servicedefaults misses LocalConnectTimeoutMs and LocalRequestTimeoutMs
6 participants