Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

Clusterbootstrap_controller should watch providers in core packages #3440

Merged
merged 2 commits into from
Sep 23, 2022

Conversation

vijaykatam
Copy link
Contributor

@vijaykatam vijaykatam commented Sep 20, 2022

  • Update watches to include providers in core packages

Signed-off-by: Vijay Katam vkatam@vmware.com

What this PR does / why we need it

Update watches to include providers in core packages so that status updates are immediately propagated

Which issue(s) this PR fixes

Fixes #3477

Describe testing done for PR

  • Added env test

Release note

package based lcm: Clusterbootstrap_controller should watch providers in core packages

Additional information

Special notes for your reviewer

* Update watches to include providers in core packages

Signed-off-by: Vijay Katam <vkatam@vmware.com>
Copy link
Contributor

@12345lcr 12345lcr left a comment

Choose a reason for hiding this comment

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

Changes LGTM, thanks!

}
err := k8sClient.Create(ctx, ns)
if err != nil {
Expect(apierrors.IsAlreadyExists(err)).To(BeTrue())
Copy link
Contributor

Choose a reason for hiding this comment

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

Since BeforeEach run on every start of It, perhaps combine the 2 It into 1 and separate with By can avoid this duplicate creation error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I combined the 2 tests into one.

Signed-off-by: Vijay Katam <vkatam@vmware.com>
@codecov
Copy link

codecov bot commented Sep 23, 2022

Codecov Report

Merging #3440 (d92a034) into main (6ab1811) will increase coverage by 0.95%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##             main    #3440      +/-   ##
==========================================
+ Coverage   52.94%   53.90%   +0.95%     
==========================================
  Files         103      105       +2     
  Lines       10419    10664     +245     
==========================================
+ Hits         5516     5748     +232     
- Misses       4443     4450       +7     
- Partials      460      466       +6     
Impacted Files Coverage Δ
addons/controllers/clusterbootstrap_controller.go 62.88% <25.00%> (-0.05%) ⬇️
cmd/cli/plugin/pinniped-auth/#.go
cmd/cli/plugin/pinniped-auth/main.go
...re-template-resolver/templateresolver/interface.go 100.00% <0.00%> (ø)
...ter/vsphere-template-resolver/template/resolver.go 89.91% <0.00%> (ø)
...emplate-resolver/templateresolver/resolver_impl.go 99.02% <0.00%> (ø)
...sphere-template-resolver/templateresolver/types.go 42.10% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@vijaykatam vijaykatam added the ok-to-merge PRs should be labelled with this before merging label Sep 23, 2022
@vijaykatam vijaykatam merged commit 78eab31 into vmware-tanzu:main Sep 23, 2022
chandrareddyp pushed a commit that referenced this pull request Sep 25, 2022
…3440)

* Clusterbootstrap_controller should watch providers in core packages

* Update watches to include providers in core packages

Signed-off-by: Vijay Katam <vkatam@vmware.com>

* Fix test and update make target for verbose tests

Signed-off-by: Vijay Katam <vkatam@vmware.com>

Signed-off-by: Vijay Katam <vkatam@vmware.com>
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clusterbootstrap controller should watch providers in core packages
4 participants