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

Fix for blank line in graffiti file #6635

Merged
merged 3 commits into from
Dec 13, 2024

Conversation

shayanb
Copy link

@shayanb shayanb commented Nov 30, 2024

Issue Addressed

Which issue # does this PR address?
#5880

Proposed Changes

Please list or describe the changes introduced by this PR.
Skip empty line as described in the issue 5880

Additional Info

Please provide any additional information. For example, future considerations
or information useful for reviewers.

@CLAassistant
Copy link

CLAassistant commented Nov 30, 2024

CLA assistant check
All committers have signed the CLA.

@michaelsproul
Copy link
Member

Hi @shayanb, do you think you could try to add a test too? You can extend or copy the existing test in that file

@michaelsproul michaelsproul added the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Dec 1, 2024
@shayanb
Copy link
Author

shayanb commented Dec 1, 2024

@michaelsproul here you go, added tests for different new line scenarios. Let me know if anything else is needed here.

@shayanb
Copy link
Author

shayanb commented Dec 1, 2024

I'd like to add printing the graffiti in the logs for validator initialization, would it fit in this PR or I can create a new PR for that.

Basically this line in validator_client/initialized_validators/src/lib.rs (although not sure how to test it yet):

                                self.validators
                                    .insert(init.voting_public_key().compress(), init);
                                info!(
                                    self.log,
                                    "Enabled validator";
                                    "signing_method" => "local_keystore",
                                    "voting_pubkey" => format!("{:?}", def.voting_public_key),
                                    **"graffiti" => format!("{:?}", def.graffiti)**
                                );

@michaelsproul
Copy link
Member

@shayanb Thanks for the test, looks good!

I think we could add the graffiti logging in a separate PR. There's some subtlety there because the graffiti can change at runtime, so whatever we log on startup won't necessarily stay. It could still be nice to log it though

@michaelsproul
Copy link
Member

Cargo fmt is failing, but you can fix it with cargo fmt --all (you can probably also configure your editor to run cargo fmt on save).

@shayanb
Copy link
Author

shayanb commented Dec 2, 2024

@michaelsproul thanks for the tip, pushed the fmt commit.

I'll submit graffiti logging PR soon.

@michaelsproul michaelsproul added val-client Relates to the validator client binary bug Something isn't working v6.1.0 New release c. Q1 2025 labels Dec 2, 2024
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Looks great, thanks

The CI failure is unrelated, will be fixed by:

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Dec 2, 2024
@michaelsproul
Copy link
Member

@mergify queue

Copy link

mergify bot commented Dec 3, 2024

queue

🛑 The pull request has been removed from the queue default

The merge conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@michaelsproul
Copy link
Member

@mergify requeue

Copy link

mergify bot commented Dec 13, 2024

requeue

✅ This pull request will be re-embarked automatically

The followup queue command will be automatically executed to re-embark the pull request

Copy link

mergify bot commented Dec 13, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 943716b

mergify bot added a commit that referenced this pull request Dec 13, 2024
@mergify mergify bot merged commit 943716b into sigp:unstable Dec 13, 2024
28 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready-for-merge This PR is ready to merge. v6.1.0 New release c. Q1 2025 val-client Relates to the validator client binary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants