-
Notifications
You must be signed in to change notification settings - Fork 95
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
install: Deploy peerpod-ctrl by default #1665
install: Deploy peerpod-ctrl by default #1665
Conversation
The default value of RESOURCE_CTRL is set to true to allow deletion of dangling resources. Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
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.
LGTM
Thank you @bpradipt |
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 some something to make the e2e tests also deploy the peerpod-ctrl?
Maybe after
cloud-api-adaptor/test/provisioner/provision.go
Lines 218 to 221 in 827d0f1
log.Info("Install the cloud-api-adaptor") | |
if err := p.installOverlay.Apply(ctx, cfg); err != nil { | |
return err | |
} |
Maybe we should add a line to the manual install path of the readme as well to tie up those loose ends?
Yeah good point. I'll look into how to enable it in e2e by default |
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.
The provisioning update code looks okay to me. Sorry to be a pain, but should we consider running peerpod-ctrl undeploy somewhere around
ccPods, err := GetDaemonSetOwnedPods(ctx, cfg, p.ccDaemonSet) |
7b533ba
to
fe59a64
Compare
Deploy peerpod-ctrl by default as part of e2e test Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
fe59a64
to
9d7bf1d
Compare
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.
Code LGTM, so as long as the e2e libvirt tests pass, I'm happy for this to be merged. Thanks @bpradipt for the code and updates.
The e2e tests are failing with:
this looks like the same issue that @karmab fixed a couple of weeks ago, though 😕 |
hi @bpradipt ! In order to test peerpod-ctrl changes on pull request you will need to build and push its image like we do for podvm and cloud-api-adaptor images. Otherwise it will be testing |
let me check |
@wainersm that's not the intent and frankly out of scope for this PR. peerpod-ctrl is treated like the operator here. |
Alright, thanks for the explanation! |
the issue is that github change again their commits page format (with no mention nowhere...), so I fixed it with https://github.com/karmab/kcli/actions/runs/7573926453 so that
|
8f0abea
into
confidential-containers:main
The default value of RESOURCE_CTRL is set to true to allow deletion of dangling resources.