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

Added required --force-bls-withdrawal-credentials description to --disable-deposits usage #6436

Merged
merged 9 commits into from
Oct 10, 2024

Conversation

hopinheimer
Copy link

fixes #6391

@CLAassistant
Copy link

CLAassistant commented Sep 25, 2024

CLA assistant check
All committers have signed the CLA.

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.

You'll also need to rebuild the CLI docs in the book using ./scripts/cli.sh

@michaelsproul michaelsproul added the docs Documentation label Sep 26, 2024
```

<style> .content main {max-width:88%;} </style>
```n<style> .content main {max-width:88%;} </style>
Copy link
Member

Choose a reason for hiding this comment

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

this looks wrong, did the script do this?

Copy link
Member

Choose a reason for hiding this comment

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

which OS did you run it on?

Copy link
Author

@hopinheimer hopinheimer Sep 26, 2024

Choose a reason for hiding this comment

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

ran a make cli-local

Copy link
Author

Choose a reason for hiding this comment

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

mac os

Copy link
Author

@hopinheimer hopinheimer Sep 26, 2024

Choose a reason for hiding this comment

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

yeah I thought something was off but the scripts/cli.sh had this

Screenshot 2024-09-26 at 12 10 11 AM

Copy link
Member

Choose a reason for hiding this comment

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

that regexp might need a tweak? it looks the same as the Linux one?

Copy link
Author

Choose a reason for hiding this comment

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

well today I learned I'm still a noob programmer, but yeah I was defeated by a linux command.
I think using a printf just like the command to add the code block quote right above this line just solves the problem in an easy way and sed is just a little ambiguous for different distros.

there's another issue I'd like to point out

Screenshot 2024-09-26 at 9 39 40 AM

when I run make cli which uses docker this happens, if that would have worked I would have not come across the current issue. do you want to create a separate tracking issue for that?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I don't think that image is ever going to be public. Maybe that's just a doc issue

Copy link
Member

Choose a reason for hiding this comment

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

Oh, it is public but it doesn't work on ARM.

Copy link
Member

Choose a reason for hiding this comment

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

If you have Rosetta you can run x86_64 images on macOS using emulation. It's slower but it works.

scripts/cli.sh Outdated
@@ -16,7 +16,7 @@ write_to_file() {
printf "# %s\n\n\`\`\`\n%s\n\`\`\`" "$program" "$cmd" > "$file"

# Adjust the width of the help text and append to the end of file
sed -i -e '$a\'$'\n''\n''<style> .content main {max-width:88%;} </style>' "$file"
printf '%s\n\n' '' '<style> .content main {max-width:88%;} </style>' >> "$file"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
printf '%s\n\n' '' '<style> .content main {max-width:88%;} </style>' >> "$file"
printf '%s\n' '' '<style> .content main {max-width:88%;} </style>' >> "$file"

I think we might want to remove the 2nd newline. The markdown linter isn't happy

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.

Markdown linter needs appeasing

scripts/cli.sh Outdated
@@ -16,7 +16,7 @@ write_to_file() {
printf "# %s\n\n\`\`\`\n%s\n\`\`\`" "$program" "$cmd" > "$file"

# Adjust the width of the help text and append to the end of file
sed -i -e '$a\'$'\n''\n''<style> .content main {max-width:88%;} </style>' "$file"
printf '%s\n\n' '' '<style> .content main {max-width:88%;} </style>' >> "$file"
Copy link
Member

@chong-he chong-he Sep 27, 2024

Choose a reason for hiding this comment

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

For consistency, may I suggest we use double quote "" (as in the code above)? We can also remove the '' in the middle of the line for simplicity.
The following should do it:

Suggested change
printf '%s\n\n' '' '<style> .content main {max-width:88%;} </style>' >> "$file"
printf "\n\n%s\n" "<style> .content main {max-width:88%;} </style>" >> "$file"

The \n\n is to create an empty line after the code block

Copy link
Author

Choose a reason for hiding this comment

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

I've made the requested changes @chong-he

@jimmygchen jimmygchen added the ready-for-review The code is ready for review label Oct 3, 2024
@chong-he
Copy link
Member

chong-he commented Oct 3, 2024

This looks good now, all checks passed

@hopinheimer
Copy link
Author

can this be merged? I'm using the changes in a different PR and yes I will shift to ubuntu soon :)

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@jxs
Copy link
Member

jxs commented Oct 9, 2024

@Mergifyio queue

Copy link

mergify bot commented Oct 9, 2024

queue

🛑 Branch protection settings are not validated anymore

Branch protection is enabled and is preventing Mergify to merge the pull request. Mergify will merge when branch protection settings validate the pull request once again. (detail: 1 review requesting changes and 1 approving review by reviewers with write access.)

@AgeManning
Copy link
Member

@Mergifyio queue

Copy link

mergify bot commented Oct 9, 2024

queue

🛑 The pull request has been removed from the queue default

Pull request #6436 has been dequeued. Branch protection settings are not validated anymore. Branch protection is enabled and is preventing Mergify to merge the pull request. Mergify will merge when branch protection settings validate the pull request once again. (detail: 1 review requesting changes and 1 approving review by reviewers with write access.).

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.

@AgeManning
Copy link
Member

@michaelsproul - I think you might need to unblock this

@jxs
Copy link
Member

jxs commented Oct 10, 2024

@Mergifyio queue

Copy link

mergify bot commented Oct 10, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at da290e8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation ready-for-review The code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants