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

feat: emit the cluster profile objects #941

Merged
merged 4 commits into from
Nov 13, 2024

Conversation

ryanzhang-oss
Copy link
Contributor

@ryanzhang-oss ryanzhang-oss commented Oct 31, 2024

Description of your changes

Fixes #467

I have:

  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Special notes for your reviewer

@ryanzhang-oss ryanzhang-oss marked this pull request as draft October 31, 2024 00:36
@ryanzhang-oss ryanzhang-oss force-pushed the cluster-profile branch 2 times, most recently from 4ebf5ea to b9aa442 Compare November 8, 2024 00:25
@ryanzhang-oss ryanzhang-oss marked this pull request as ready for review November 8, 2024 00:29
Copy link
Contributor

@michaelawyu michaelawyu left a comment

Choose a reason for hiding this comment

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

Added some minor comments, PTAL 🙏

)

// This container cannot be run in parallel with other ITs because it uses a shared fakePlacementController.
var _ = Describe("Test ClusterProfile Controller", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Ryan! A nit: I understand that currently there are no other integration tests, but maybe we should mark it as Serial already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will this suite interfere with other integration tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, the comment (line 33) mentions so; I assume it's because of the member cluster creation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, the comment is wrong

if condition == nil {
t.Fatalf("expected condition to be set, but it was not")
}
if condition.Status != tt.expectedConditionStatus {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Ryan! It might be easier to use the cmp package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will open another PR

Copy link
Contributor

@zhiying-lin zhiying-lin left a comment

Choose a reason for hiding this comment

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

code LGTM.

@ryanzhang-oss ryanzhang-oss merged commit 6202f38 into Azure:main Nov 13, 2024
12 checks passed
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.

[feature] adopt cluster inventory API from k8s-sig multi-cluster
3 participants