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

Drop the events system #468

Merged
merged 3 commits into from
Apr 8, 2021
Merged

Conversation

gauravgahlot
Copy link
Contributor

Description

The PR removes all the code and DB changes related to the events system.

Why is this needed

Fixes: #464

How Has This Been Tested?

  • Existing unit tests
  • Manual tests

How are existing users impacted? What migration steps/scripts do we need?

This is a breaking change, especially for any users currently using events.
If your tool is not using tink events the migration should be smooth.

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

@gauravgahlot gauravgahlot requested a review from gianarb March 26, 2021 10:22
@gauravgahlot gauravgahlot self-assigned this Mar 26, 2021
@gauravgahlot gauravgahlot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Mar 26, 2021
@codecov
Copy link

codecov bot commented Mar 26, 2021

Codecov Report

Merging #468 (40f69bb) into master (838453e) will decrease coverage by 0.22%.
The diff coverage is 100.00%.

❗ Current head 40f69bb differs from pull request most recent head ce914db. Consider uploading reports for the commit ce914db to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #468      +/-   ##
==========================================
- Coverage   32.32%   32.09%   -0.23%     
==========================================
  Files          50       44       -6     
  Lines        3276     3103     -173     
==========================================
- Hits         1059      996      -63     
+ Misses       2123     2017     -106     
+ Partials       94       90       -4     
Impacted Files Coverage Δ
cmd/tink-cli/cmd/hardware.go 73.33% <ø> (-1.67%) ⬇️
db/db.go 52.63% <ø> (+7.17%) ⬆️
db/migration/migration.go 100.00% <ø> (ø)
grpc-server/grpc_server.go 0.00% <ø> (ø)
db/migration/2021032610300-drop-events-system.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 838453e...ce914db. Read the comment docs.

@gianarb gianarb added breaking-change Denotes a PR that introduces potentially breaking changes that require user action. ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ labels Mar 26, 2021
@gianarb gianarb requested review from detiber and removed request for gianarb March 26, 2021 13:50
@gianarb
Copy link
Contributor

gianarb commented Mar 26, 2021

@detiber do you mind to have a look at this please?

@gianarb gianarb added ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ and removed ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ labels Mar 29, 2021
@gauravgahlot gauravgahlot added ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ and removed ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ labels Mar 29, 2021
@gianarb gianarb added ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ and removed ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ labels Mar 30, 2021
Signed-off-by: Gaurav Gahlot <[email protected]>
Signed-off-by: Gaurav Gahlot <[email protected]>
Signed-off-by: Gaurav Gahlot <[email protected]>
@gauravgahlot gauravgahlot removed the ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ label Apr 1, 2021
@moadqassem
Copy link
Member

@gauravgahlot So once this PR is merged, the 8000 bytes limit from the triggers will be solved right?

@gauravgahlot
Copy link
Contributor Author

gauravgahlot commented Apr 1, 2021

@gauravgahlot So once this PR is merged, the 8000 bytes limit from the triggers will be solved right?

@moadqassem After this PR is merged there will be no event triggers at all. 😄

Copy link
Contributor

@detiber detiber left a comment

Choose a reason for hiding this comment

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

lgtm

@gianarb gianarb added the ready-to-merge Signal to Mergify to merge the PR. label Apr 8, 2021
@mergify mergify bot merged commit a7ffd35 into tinkerbell:master Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Denotes a PR that introduces potentially breaking changes that require user action. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the event system
4 participants