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

cmd: fixing builder registration timestamp #2810

Merged
merged 2 commits into from
Jan 22, 2024
Merged

cmd: fixing builder registration timestamp #2810

merged 2 commits into from
Jan 22, 2024

Conversation

pinebit
Copy link
Contributor

@pinebit pinebit commented Jan 22, 2024

Use the current timestamp for builder registration messages when using --split-existing-keys mode.

See the issue for the full context.

category: bug
ticket: #2770

@pinebit pinebit changed the title cmd: builder registration to use the current timestamp for --split-existing-keys mode. cmd: builder registration to use the current timestamp when splitting keys Jan 22, 2024
@codecov Codecov
Copy link

codecov bot commented Jan 22, 2024 •
edited
Loading

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (4e16691) 53.28% compared to head (2c5289a) 53.24%.
Report is 1 commits behind head on main.

Files Patch % Lines
cmd/createcluster.go 76.92% 2 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2810      +/-   ##
==========================================
- Coverage   53.28%   53.24%   -0.05%     
==========================================
  Files         199      199              
  Lines       27691    27696       +5     
==========================================
- Hits        14756    14747       -9     
- Misses      11108    11122      +14     
  Partials     1827     1827

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pinebit pinebit changed the title cmd: builder registration to use the current timestamp when splitting keys cmd: builder registration to use time.Now() when splitting keys Jan 22, 2024
@pinebit pinebit changed the title cmd: builder registration to use time.Now() when splitting keys cmd: fixing builder registration timestamp Jan 22, 2024
@pinebit pinebit mentioned this pull request Jan 22, 2024
Merged
gsora
gsora approved these changes Jan 22, 2024
View reviewed changes
cmd/createcluster_internal_test.go
// For SplitKeys mode, builder registration timestamp must be close to Now().
// This assumes the test does not execute longer than five minutes.
// We just need to make sure the message timestamp is not a genesis time.
require.Less(t, nowUTC.Sub(val.BuilderRegistration.Message.Timestamp), 5*time.Minute, "likely a genesis timestamp")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm, considering our CI isn't the fastest, I wonder if we'll ever reach a state in which this statement fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is fast, it does not deploy and run a cluster. On a dev box it is executing in ~0.2 sec. Hence, the assumption about 5 minutes to be extremely sufficient..

dB2510
dB2510 approved these changes Jan 22, 2024
View reviewed changes
Copy link
Contributor

@dB2510 dB2510 left a comment

Choose a reason for hiding this comment

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

LGTM, just a small suggestion.

cmd/createcluster.go Outdated Show resolved Hide resolved
cmd/createcluster.go Outdated Show resolved Hide resolved
cmd/createcluster.go Outdated Show resolved Hide resolved
cmd/createcluster.go Outdated Show resolved Hide resolved
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jan 22, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@pinebit pinebit added the merge when ready Indicates bulldozer bot may merge when all checks pass label Jan 22, 2024
@obol-bulldozer obol-bulldozer bot merged commit dba09ba into main Jan 22, 2024
13 checks passed
@obol-bulldozer obol-bulldozer bot deleted the pinebit/split-keys-to-use-now branch January 22, 2024 15:11
gsora pushed a commit that referenced this pull request Feb 21, 2024
@pinebit @gsora
cmd: fixing builder registration timestamp (#2810)
8e6a305 
Use the current timestamp for builder registration messages when using `--split-existing-keys` mode.

See [the issue](#2770) for the full context.

category: bug
ticket: #2770
@gsora gsora mentioned this pull request Feb 21, 2024
Merged
obol-bulldozer bot pushed a commit that referenced this pull request Feb 21, 2024
@gsora
cmd: fixing builder registration timestamp (#2892)
3c5fdad 
Cherrypick of #2810 

---

Use the current timestamp for builder registration messages when using `--split-existing-keys` mode.

See [the issue](#2770) for the full context.

category: bug
ticket: #2770
@boulder225 boulder225 mentioned this pull request Mar 30, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gsora gsora gsora approved these changes

@dB2510 dB2510 dB2510 approved these changes

@OisinKyne OisinKyne Awaiting requested review from OisinKyne

@xenowits xenowits Awaiting requested review from xenowits

Assignees
No one assigned
Labels
merge when ready Indicates bulldozer bot may merge when all checks pass
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pinebit @gsora @dB2510