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

[refactor] move code from AlertsController to AlertService #2435

Merged
merged 24 commits into from
Aug 5, 2024

Conversation

pwallk
Copy link
Contributor

@pwallk pwallk commented Aug 1, 2024

What's changed?

Checklist

  • I have read the Contributing Guide
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

Add or update API

  • I have added the necessary e2e tests and all cases have passed.

@Calvin979 Calvin979 added the good first pull request Good for newcomers label Aug 1, 2024
&& argument.getPageSize() == pageRequest.getPageSize()
&& argument.getSort().equals(pageRequest.getSort())
)
ids, 1L, (byte) 1, (byte) 1, "test", sortField, orderType, pageIndex, pageSize
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice work! Could you help fix this test. Unit tests are passed because this test method is not annotated with @Test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

@@ -18,7 +18,7 @@ spring:
application:
name: ${HOSTNAME:@hertzbeat@}${PID}
profiles:
active: prod
active: pg
Copy link
Contributor

Choose a reason for hiding this comment

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

Why modify application.yml? Looks like it is a dev configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, forgot to rollback

pwallk and others added 3 commits August 5, 2024 09:26
# Conflicts:
#	alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/AlertServiceImpl.java
Copy link
Contributor

@Calvin979 Calvin979 left a comment

Choose a reason for hiding this comment

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

lgtm

@Calvin979 Calvin979 merged commit e560502 into apache:master Aug 5, 2024
3 checks passed
@pwallk pwallk deleted the refactor-AlertsController branch August 15, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants