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

Add new Accepted tests in hertzbeat #1149

Merged
merged 1 commit into from
Dec 3, 2023

Conversation

bbelide2
Copy link
Contributor

@bbelide2 bbelide2 commented Dec 2, 2023

Adding 2 new Accepted tests in hertzbeat.

Both the tests are ID: details and logs are present in #1148
Root cause and fix details are present in the PRs:

apache/hertzbeat#1373
apache/hertzbeat#1371

@@ -467,4 +468,4 @@
"https://github.com/zeroturnaround/gradle-jrebel-plugin": "unforked",
"https://github.com/zhangxd1989/spring-boot-cloud": "unforked",
"https://github.com/zhurlik/gradle-jboss-modules-plugin": "unforked"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What editor/IDE are you using to get this change automatically? At some point we may accept that change to stop dealing with unnecessary changes.

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 usually use vs code but for this change I directly made in GitHub by editing the file

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you test on another file if the GitHub editor automatically removes or adds newline at the end of the file?

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 edited another file requirements.txt. I just removed 1 and added 1 in the last line. That line is shown in the diff despite being exactly same.

Copy link
Contributor

Choose a reason for hiding this comment

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

The line is not the same if it's in the diff. Can you search online about reports of GitHub editor adding/removing the newline character? Can that behavior be customized?

Copy link
Contributor Author

@bbelide2 bbelide2 Dec 2, 2023

Choose a reason for hiding this comment

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

I searched online and in github settings, but did not find any setting to control new lines in the github web editor. But there are options to manage these configurations at IDE level like in VS code. Also, there is an option in GitHub to edit a file in github.dev which opens the repo in an online vs code editor.

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 think that the GitHub editor is adding a new line at the end if the file doesn't have one

Copy link
Contributor Author

@bbelide2 bbelide2 Dec 2, 2023

Choose a reason for hiding this comment

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

In this PR, I was not able to reproduce the exact diff in new branches, I may have inadvertantly changed some space

Copy link
Contributor

Choose a reason for hiding this comment

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

I found https://stackoverflow.com/questions/68088682/how-do-i-stop-githubs-web-editor-from-adding-a-newline-to-the-end-of-file but didn't test anything.

Please remove unnecessary changes so I can accept your PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed unnecessary changes

@darko-marinov
Copy link
Contributor

Please squash commits.

@bbelide2 bbelide2 force-pushed the hertz branch 3 times, most recently from 6b757a4 to 1d5ea7e Compare December 3, 2023 20:29
@bbelide2
Copy link
Contributor Author

bbelide2 commented Dec 3, 2023

Please squash commits.

Squashed all the commits and merged new changes

@darko-marinov
Copy link
Contributor

I still see 2 commits.

@bbelide2
Copy link
Contributor Author

bbelide2 commented Dec 3, 2023

I squashed all the commits into one.

@darko-marinov
Copy link
Contributor

@zzjas can you please double check? Things looked fine to me, but the PR is so big that it's good to have two reviewers.

@bbelide2
Copy link
Contributor Author

bbelide2 commented Dec 3, 2023

Contributor

I added only 2 tests, but the diff is showing all the changes done by commits in between as well. I am not sure why but when I tried to squash, I am getting all the changes made by other people in between as well. Should I close this PR and open a new fresh PR instead?

@darko-marinov
Copy link
Contributor

No, please don't close; you'll get -2 "points" for that. Just learn how to properly rebase your work and squash your commits. Please ask on Campuswire if you need help.

@bbelide2
Copy link
Contributor Author

bbelide2 commented Dec 3, 2023

No, please don't close; you'll get -2 "points" for that. Just learn how to properly rebase your work and squash your commits. Please ask on Campuswire if you need help.

I tried to do that but I am getting 2 commits - one squashed commit and one merge commit. This way I am only getting my changes. I did that now. I am having 2 commits including a merge commit.

@bbelide2 bbelide2 reopened this Dec 3, 2023
@darko-marinov
Copy link
Contributor

Thanks for cleaning this even while your laptop is broken!

@darko-marinov darko-marinov merged commit 34c96f8 into TestingResearchIllinois:main Dec 3, 2023
2 checks passed
@bbelide2 bbelide2 deleted the hertz branch December 4, 2023 00:04
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.

2 participants