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.

Sign up for GitHub

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

Jump to bottom

feat: add Licensei #696

Closed
wants to merge 2 commits into from
Closed

feat: add Licensei #696

wants to merge 2 commits into from

Conversation

csatib02
Copy link

@csatib02 csatib02 commented Mar 15, 2024 •
edited
Loading

Overview

  • Connected Licensei to the CI.
  • Added license-header to all go files.
  • Fixed all license-check errors.
  • Updated the readme.

Fixes #22

@sagikazarmark sagikazarmark added the release-note/misc Miscellaneous changes label Mar 18, 2024
sagikazarmark
sagikazarmark suggested changes Mar 18, 2024
View reviewed changes
Copy link
Contributor

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

Can you please take a look at the failing tests?

@tothandras
Copy link
Contributor

@csatib02 Please check the failing test, then break it into two commits, so it's easier to review: 1. configs, readme, etc 2. license headers

@csatib02 csatib02 force-pushed the feat/add-licensei-and-license-header-to-all-files branch from 94ea8ff to cc1bd18 Compare March 18, 2024 11:13
@csatib02
Copy link
Author

csatib02 commented Mar 18, 2024 •
edited
Loading

I looked into the flaky e2e-tests, and it is most probably linked to the timing of the events created.

In my case the e2e-test showed, that the results returned by the query had the values: 1000, and 1000, but 1500 and 500 were expected.
So 10 events fell into each window.

  • The first 5 events are generated at the initial timestamp, then the next five at initial timestamp plus one minute, then plus one hour, and finally plus one day.
  • At test "Day", it queries for the sum of events happened in two 24h windows:
  1. Starting at the initial timestamp
  • 3 event generations should fall in here: initial, plus one minute, plus one hour.
  • A total of 15 events expected.
  1. Then at initial timestamp + 24 hours
  • 1 event generation should happen at this point: plus one day.
  • A total of 5 events expected.

The reason why it fails sometimes:

  • Despite of the fact that initial timestamp is seeded, it can change when a new dagger engine is hosted. (I added some pictures of logs. I managed to produce a fail aswell. :)
  • If the initial timestamp is close to the end of a 24hour window, it can end up in the next window. ( In case of initial timestamp plus one minute, or one hour)

I can come up with a solution for this in a follow up PR if you want me to do so. 😀
I opened an issue regarding this: #707

Pass
image

Fail
image

@csatib02 csatib02 mentioned this pull request Mar 18, 2024
Open
2 tasks
@csatib02 csatib02 requested a review from sagikazarmark March 18, 2024 18:17
@sagikazarmark
Copy link
Contributor

Can you please rebase?

@csatib02 csatib02 force-pushed the feat/add-licensei-and-license-header-to-all-files branch 2 times, most recently from 11dfbbb to c2fdb7d Compare May 24, 2024 16:26
@csatib02
Copy link
Author

@sagikazarmark Done.

@csatib02 csatib02 force-pushed the feat/add-licensei-and-license-header-to-all-files branch 3 times, most recently from e664444 to 3d22d80 Compare May 28, 2024 10:22
@csatib02 csatib02 force-pushed the feat/add-licensei-and-license-header-to-all-files branch 2 times, most recently from ae74eee to 3d23277 Compare July 1, 2024 08:58
@csatib02 csatib02 force-pushed the feat/add-licensei-and-license-header-to-all-files branch 10 times, most recently from 7a3e29d to 5a1572c Compare August 7, 2024 07:23
@csatib02 csatib02 force-pushed the feat/add-licensei-and-license-header-to-all-files branch 3 times, most recently from 946d791 to 47b5263 Compare August 16, 2024 10:44
@csatib02 csatib02 force-pushed the feat/add-licensei-and-license-header-to-all-files branch from 47b5263 to 6e18505 Compare August 25, 2024 10:30
@csatib02 csatib02 force-pushed the feat/add-licensei-and-license-header-to-all-files branch from 6e18505 to e5f06c6 Compare August 25, 2024 10:41
@csatib02
configs and readme
8d18c32 
Signed-off-by: Bence Csati <[email protected]>
@csatib02 csatib02 force-pushed the feat/add-licensei-and-license-header-to-all-files branch from e5f06c6 to afe5b8f Compare September 21, 2024 08:46
@csatib02
feat: add license headers
f890ab4 
Signed-off-by: Bence Csati <[email protected]>
@csatib02 csatib02 force-pushed the feat/add-licensei-and-license-header-to-all-files branch from afe5b8f to f890ab4 Compare September 21, 2024 08:48
@GAlexIHU
Copy link
Contributor

Closing due to inactivity

@GAlexIHU GAlexIHU closed this Nov 15, 2024
@csatib02
Copy link
Author

Hey @GAlexIHU,

This PR was opened nearly a year ago...
Despite my best efforts, I barely even got a response on it, although i rebased it like a 100 times. :D

I would still finish-up this one, it depends whether you are interested in it.

@GAlexIHU
Copy link
Contributor

Hey @csatib02, thanks for the continued interest in contributing!

I'm personally not sure what the current take is on Licensei integration / source bleeding, probably @tothandras or @turip has more insight on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sagikazarmark sagikazarmark Awaiting requested review from sagikazarmark

Assignees
No one assigned
Labels
release-note/misc Miscellaneous changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add license header to all files
4 participants
@csatib02 @tothandras @sagikazarmark @GAlexIHU