-
Notifications
You must be signed in to change notification settings - Fork 212
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
Improve proto generation #5312
Improve proto generation #5312
Conversation
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
aks-node-controller/pkg/gen/aksnodeconfig/v1/tls_bootstrapping_config.pb.go
Outdated
Show resolved
Hide resolved
aks-node-controller/README.md
Outdated
note over VM: cloud-init handles<br/>config.json deployment | ||
|
||
note over VM: cloud-init completes processing | ||
note over VM: Start aks-node-controller.service (systemd service)<br/> afer cloud-init |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo after
.
Thanks for reworking the diagram, much easier to follow.
aks-node-controller/README.md
Outdated
@@ -4,7 +4,7 @@ This directory contains files related to AKS Node Controller go binary. | |||
|
|||
## Overview | |||
|
|||
AKS Node Controller is a go binary that is responsible for bootstrapping AKS nodes. The controller expects a predefined contract from the client of type [`AKSNodeConfig`](https://github.com/Azure/AgentBaker/tree/dev/pkg/proto/aksnodeconfig/v1) containing the bootstrap configuration. The controller has two primary functions: 1. Parse the bootstrap config and kickstart bootstrapping and 2. Monitor the completion status. | |||
AKS Node Controller is a go binary that is responsible for bootstrapping AKS nodes. The controller expects a predefined contract from the client of type [`AKSNodeConfig`](https://github.com/Azure/AgentBaker/tree/dev/pkg/proto/aksnodeconfig/v1) containing the bootstrap configuration. The controller has two primary functions: 1. Parse the bootstrap config and kickstart bootstrapping and 2. Monitor the completion status. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the paragraph below to remove the reference to GetNodeBootstrapping
now that it is outdated?
# Emulate running "buf" in the current directory | ||
BUF = docker run --volume "$(CURDIR)/../:$(CURDIR)/../" --workdir $(CURDIR) bufbuild/buf:1.47.2 | ||
|
||
.PHONY: proto-generate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to manually run make proto-generate
before pushing the commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, CI may complain about it. But I didn't test it.
.PHONY: proto-lint | ||
proto-lint: | ||
@($(BUF) lint) | ||
@($(BUF) breaking --against '../.git#branch=dev,subdir=aks-node-controller') # TODO: change to master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lint: Not sure if there is a good way to point to default
so that we don't need to switch between master and dev every year.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couldn't find anything in the buf docs, I don't think it's supported. But CI behaves differently, it checks target branch for PRs.
|
||
CSE script: `/opt/azure/containers/aks-node-controller provision-wait` | ||
|
||
|
||
#### Provisioning flow diagram: | ||
|
||
 | ||
```mermaid | ||
sequenceDiagram |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great we can 'write' a diagram!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved with question
/kind cleanup
Note: I didn't test it on amd64. May need to adjust dockerfile if I put an incorrect value there.