-
Notifications
You must be signed in to change notification settings - Fork 215
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
chore: bump crictl-tools 1.32.0 #6076
Conversation
lgtm pending upstream build |
fyi |
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
No changes to cached containers or packages on Windows VHDs |
@@ -6,7 +6,7 @@ BUF = docker run --volume "$(CURDIR)/../:$(CURDIR)/../" --workdir $(CURDIR) bufb | |||
proto-generate: | |||
@($(BUF) format -w) | |||
rm -rf pkg/gen/aksnodeconfig/v1 | |||
docker build --platform $(shell uname -m) -t protoc-docker - < protoc.Dockerfile | |||
docker build --platform linux/amd64 -t protoc-docker - < protoc.Dockerfile |
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.
Why do we need this change? I am not sure if they are equivalent because when I run this command on my WSL, I get a different result.
root@DESKTOP-1TJD8FB:~/git/AgentBaker/bugbash/proxytest# uname -m
x86_64
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.
When I was running make proto-generate
it would fail due my mac being arm64. Seems that
FROM mcr.microsoft.com/azurelinux/base/core:3.0
from protc.Dockerfile requires linux/amd64? So I think it makes sense to explicitly set that?
If you run make proto-generate
does it still work for you for x86_64?
Also I was relying DIYing geting the changes to scriptless (since it seems like there wasn't a way to update the code to run the e2e's manually?) So I might have not have followed the correct flow
@@ -150,4 +150,7 @@ message Configuration { | |||
|
|||
// IMDS restriction configuration | |||
ImdsRestrictionConfig imds_restriction_config = 39; | |||
|
|||
// Kubelet Config File Content | |||
string kubelet_config_file_content = 40; |
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.
We do have this in the KubeletConfig. If the PR gate tests failed, please rebase with master and we shouldn't need to add this.
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.
When was this added? I rebased master last night and the e2e's were failing with
----------------------------------- begin stdout -----------------------------------,
{"time":"2025-03-25T09:46:51.27364042Z","level":"INFO","msg":"aks-node-controller started"}
{"time":"2025-03-25T09:47:05.397629699Z","level":"INFO","msg":"aks-node-controller started"}
{"time":"2025-03-25T09:47:05.428495257Z","level":"ERROR","msg":"aks-node-controller failed","error":"unmarshal provision config: proto: (line 1:2626): unknown field \"kubelet_config_file_content\""}
----------------------------------- end stdout ------------------------------------
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.
re-running now on a different PR with a rebase to cover my bases 🥁
What type of PR is this? updading critcl https://portal.microsofticm.com/imp/v5/incidents/details/608590533/summary
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Requirements:
Special notes for your reviewer:
Release note: