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

Add a docs link to the log message about each failing bootstrap check #99644

Merged
merged 8 commits into from
Sep 21, 2023
Merged

Add a docs link to the log message about each failing bootstrap check #99644

merged 8 commits into from
Sep 21, 2023

Conversation

NEUpanning
Copy link
Contributor

Add a docs link to the log message about each failing bootstrap check to help new users to understand failing bootstrap checks.

Closes #99614

@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v8.11.0 labels Sep 19, 2023
@romseygeek romseygeek added :Core/Infra/Node Lifecycle Node startup, bootstrapping, and shutdown and removed needs:triage Requires assignment of a team area label labels Sep 19, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Sep 19, 2023
DaveCTurner
DaveCTurner reviewed Sep 20, 2023
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner 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, I left only superficial comments.

server/src/test/java/org/elasticsearch/bootstrap/BootstrapChecksTests.java Outdated Show resolved Hide resolved
...y/src/main/java/org/elasticsearch/xpack/security/SecurityImplicitBehaviorBootstrapCheck.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/common/ReferenceDocs.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/common/ReferenceDocs.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/bootstrap/BootstrapChecks.java Outdated Show resolved Hide resolved
qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/EvilBootstrapChecksTests.java Outdated Show resolved Hide resolved
@DaveCTurner
Copy link
Contributor

@elasticmachine ok to test

@DaveCTurner DaveCTurner self-assigned this Sep 20, 2023
@DaveCTurner
Copy link
Contributor

I pushed a formatting fix and a changelog entry to your branch.

@DaveCTurner
Copy link
Contributor

@NEUpanning could you make sure your code passes ./gradlew precommit? It doesn't currently, that's why the build failed.

DaveCTurner
DaveCTurner reviewed Sep 20, 2023
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

I like it, just a few formatting comments left.

...y/src/main/java/org/elasticsearch/xpack/security/SecurityImplicitBehaviorBootstrapCheck.java Outdated
+ "."
+ Version.CURRENT.minor
+ "/security-minimal-setup.html to configure security, or explicitly disable security by "
+ "See " + this.referenceDocs() + " to configure security, or explicitly disable security by "
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

server/src/test/java/org/elasticsearch/bootstrap/BootstrapChecksTests.java Outdated
Comment on lines 168 to 169
containsString("For more information see " +
"[https://www.elastic.co/guide/en/elasticsearch/reference/")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: combine string constants (may need reformatting)

Suggested change
containsString("For more information see " +
"[https://www.elastic.co/guide/en/elasticsearch/reference/")
containsString("For more information see [https://www.elastic.co/guide/en/elasticsearch/reference/")

server/src/test/java/org/elasticsearch/bootstrap/BootstrapChecksTests.java Outdated
Comment on lines 217 to 218
assertThat(e.getMessage(), containsString("; for more information see " +
"[https://www.elastic.co/guide/en/elasticsearch/reference/"));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: combine string constants (may need reformatting)

Suggested change
assertThat(e.getMessage(), containsString("; for more information see " +
"[https://www.elastic.co/guide/en/elasticsearch/reference/"));
assertThat(e.getMessage(), containsString("; for more information see [https://www.elastic.co/guide/en/elasticsearch/reference/"));

(same comment for the other assertions added in this test suite)

DaveCTurner
DaveCTurner approved these changes Sep 20, 2023
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM (assuming CI is happy)

@NEUpanning
Copy link
Contributor Author

NEUpanning commented Sep 20, 2023 •
edited
Loading

I see that we don't have G1GC bootstrap check but there is its own page.Does we do this check in other place?

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Sep 20, 2023
@DaveCTurner
Copy link
Contributor

I see that we don't have G1GC bootstrap check

Aha good catch, this check is obsolete in 8.x and was removed in #85361 but we failed to clean up the docs.

@NEUpanning
Copy link
Contributor Author

I see that execution failed for task ':x-pack:plugin:esql:compute:test' in the Jenkins logs.But my code passes this task locally.I will try to fix it.

@DaveCTurner
Copy link
Contributor

This test is known faulty, see #99649. It's muted in latest main (see #99651) so I'll just merge that in.

@DaveCTurner
Merge remote-tracking branch 'upstream/main' into reviews/2023/09/20/…
2dd6c57 
…NEUpanning/add_docs_link_to_bootstrap_checks
@NEUpanning
Copy link
Contributor Author

NEUpanning commented Sep 21, 2023 •
edited
Loading

It seems like the reason for the build failure is #97795

@DaveCTurner
Copy link
Contributor

Yes we're seeing a little instability in CI at the moment. Don't worry about it, we'll get this merged eventually.

@DaveCTurner
Copy link
Contributor

@elasticmachine please run elasticsearch-ci/part-1

@elasticsearchmachine elasticsearchmachine merged commit c3dece1 into elastic:main Sep 21, 2023
@NEUpanning
Copy link
Contributor Author

@DaveCTurner Thanks for your review and patience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DaveCTurner DaveCTurner DaveCTurner approved these changes

Assignees

@DaveCTurner DaveCTurner

Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Core/Infra/Node Lifecycle Node startup, bootstrapping, and shutdown >enhancement Team:Core/Infra Meta label for core/infra team v8.11.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Failing bootstrap checks should link to reference docs
4 participants
@NEUpanning @elasticsearchmachine @DaveCTurner @romseygeek