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 Licensei #696

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

csatib02
Copy link

@csatib02 csatib02 commented Mar 15, 2024

Overview

  • Added makefile target for project dependencies.

Namely:

  1. Air
  2. Dagger
  3. Golangci-lint
  4. Licensei (NOTE: Was already added to nix.)
  5. Benthos
  • Connected Licensei to the CI.
  • Added license-header to all go files.
  • Fixed all license-check errors.
  • Updated the readme.

Fixes #22

Notes for reviewer

  • I haven't been able to find a license for: github.com/couchbase/goprotostellar.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@sagikazarmark sagikazarmark added the release-note/misc Miscellaneous changes label Mar 18, 2024
Copy link
Member

@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

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

@sagikazarmark
Copy link
Member

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 2 times, most recently from 4d1604a to e664444 Compare May 24, 2024 16:40
feat: Add makefile target for deps, add licensei

Signed-off-by: Bence Csati <bcsati@cisco.com>

feat: add ignored imports

Signed-off-by: Bence Csati <bcsati@cisco.com>

feat: add isc and mpl-2.0 as approved licenses

Signed-off-by: Bence Csati <bcsati@cisco.com>

feat: add more to ignored

Signed-off-by: Bence Csati <bcsati@cisco.com>

feat: add more to ignored

Signed-off-by: Bence Csati <bcsati@cisco.com>

chore: add call for print-target

Signed-off-by: Bence Csati <bcsati@cisco.com>

chore: update readme

Signed-off-by: Bence Csati <bcsati@cisco.com>

Signed-off-by: Bence Csati <bcsati@cisco.com>

chore: Add missing license ignores
Signed-off-by: Bence Csati <bcsati@cisco.com>
Signed-off-by: Bence Csati <bcsati@cisco.com>
Signed-off-by: Bence Csati <bcsati@cisco.com>
Signed-off-by: Bence Csati <bcsati@cisco.com>
Signed-off-by: Bence Csati <bcsati@cisco.com>
@csatib02 csatib02 force-pushed the feat/add-licensei-and-license-header-to-all-files branch from e664444 to 3d22d80 Compare May 28, 2024 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc Miscellaneous changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add license header to all files
3 participants