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:add a new filed EnableRating in repository #351

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

zqq454224016
Copy link
Collaborator

What type of PR is this?

What this PR does / why we need it

Which issue(s) this PR fixes

Fixes #345

Special notes for your reviewer

@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Merging #351 (6845925) into main (72eb4c0) will decrease coverage by 0.14%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #351      +/-   ##
==========================================
- Coverage   26.28%   26.14%   -0.14%     
==========================================
  Files          38       38              
  Lines        3748     3768      +20     
==========================================
  Hits          985      985              
- Misses       2691     2711      +20     
  Partials       72       72              
Files Changed Coverage Δ
api/v1alpha1/rating_types.go 100.00% <ø> (ø)
api/v1alpha1/repository_types.go 100.00% <ø> (ø)
controllers/rating_controller.go 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@zqq454224016 zqq454224016 temporarily deployed to Dev September 19, 2023 08:58 — with GitHub Actions Inactive
@zqq454224016 zqq454224016 temporarily deployed to Dev September 19, 2023 08:58 — with GitHub Actions Inactive
@zqq454224016 zqq454224016 temporarily deployed to Dev September 19, 2023 09:18 — with GitHub Actions Inactive
@zqq454224016 zqq454224016 temporarily deployed to Dev September 19, 2023 09:18 — with GitHub Actions Inactive
@zqq454224016 zqq454224016 temporarily deployed to Dev September 20, 2023 01:46 — with GitHub Actions Inactive
@zqq454224016 zqq454224016 temporarily deployed to Dev September 20, 2023 01:46 — with GitHub Actions Inactive
@zqq454224016 zqq454224016 temporarily deployed to Dev September 20, 2023 03:06 — with GitHub Actions Inactive
@zqq454224016 zqq454224016 temporarily deployed to Dev September 20, 2023 03:06 — with GitHub Actions Inactive
Copy link
Collaborator

@bjwswang bjwswang left a comment

Choose a reason for hiding this comment

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

@zqq454224016 Good job! Some tiny changes still required when doing the example-test.

config/samples/rating_test.sh Outdated Show resolved Hide resolved
@zqq454224016 zqq454224016 temporarily deployed to Dev September 20, 2023 05:49 — with GitHub Actions Inactive
@zqq454224016 zqq454224016 temporarily deployed to Dev September 20, 2023 05:49 — with GitHub Actions Inactive
bjwswang
bjwswang previously approved these changes Sep 20, 2023
if err := r.Client.Get(ctx, types.NamespacedName{Namespace: instance.Namespace, Name: component.Status.RepositoryRef.Name}, &repository); err != nil || !repository.Spec.EnableRating {
instanceDeepCopy := instance.DeepCopy()
cond := corev1alpha1.Condition{
Status: v1.ConditionFalse,
Copy link
Member

Choose a reason for hiding this comment

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

So rating's condition is only show when there are some error or rating is not enable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should update condition when

  1. error is not nil and not NotFound
  2. error is nil, but rating is disabled.

@zqq454224016

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When the rating is disable, what error is returned?

Copy link
Collaborator

Choose a reason for hiding this comment

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

A simple error message is enough just like the condition message you used here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So rating's condition is only show when there are some error or rating is not enable?

Actually we should track the whole running status in Rating.Let's do this in another pr

@zqq454224016 zqq454224016 temporarily deployed to Dev September 20, 2023 10:22 — with GitHub Actions Inactive
@zqq454224016 zqq454224016 temporarily deployed to Dev September 20, 2023 10:23 — with GitHub Actions Inactive
@zqq454224016 zqq454224016 temporarily deployed to Dev September 21, 2023 01:45 — with GitHub Actions Inactive
@zqq454224016 zqq454224016 temporarily deployed to Dev September 21, 2023 01:45 — with GitHub Actions Inactive
config/samples/rating_test.sh Show resolved Hide resolved
@bjwswang bjwswang merged commit d899a16 into kubebb:main Sep 21, 2023
@bjwswang bjwswang mentioned this pull request Sep 21, 2023
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.

3 participants