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

Show -Dlicense.key value in test repro info #66179

Merged

Conversation

breskeby
Copy link
Contributor

  • When a -Dlicense.key sys property is passed to the build we want to consider
    this in the test reproduction info message
  • absolute Paths tried to be converted to relative paths relative to workspace
    root to allow simply copy & paste
  • Also fixes a inconsistency for checking license existence in x-pack plugin core build

@breskeby breskeby self-assigned this Dec 10, 2020
@breskeby breskeby added :Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team v7.11.0 v8.0.0 labels Dec 10, 2020
@breskeby breskeby requested a review from rjernst December 10, 2020 15:53
@breskeby breskeby marked this pull request as ready for review December 10, 2020 15:53
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

@@ -181,10 +182,32 @@ private ReproduceErrorMessageBuilder appendESProperties() {
appendOpt("tests.timezone", TimeZone.getDefault().getID());
appendOpt("tests.distribution", System.getProperty("tests.distribution"));
appendOpt("runtime.java", Integer.toString(JavaVersion.current().getVersion().get(0)));
appendOpt("runtime.java", Integer.toString(JavaVersion.current().getVersion().get(0)));
Copy link
Member

Choose a reason for hiding this comment

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

This line seems to be duplicated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

appendOpt(ESTestCase.FIPS_SYSPROP, System.getProperty(ESTestCase.FIPS_SYSPROP));
return this;
}

private String relativePath(String pathProperty) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary once the fix is made in xpack to use Gradle's file resolution to get the path relative to the root project? With that change, and using a relative path in CI, shouldn't the reproduction line should just work for devs copying it from CI to their machines?

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 also works with having an absolute path passed in the commandline (e.g.on ci)

Copy link
Contributor Author

@breskeby breskeby Dec 10, 2020

Choose a reason for hiding this comment

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

But it will not required if we ensure we always pass a relative path here only

Copy link
Member

Choose a reason for hiding this comment

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

I'm just wondering if is necessary at all. It is specifically for CI. If a developer passes an absolute path, it will still work. We just need to ensure we pass a relative path in CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in that case I can remove this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

- When a -Dlicense.key sys property is passed to the build we want to consider
this in the test reproduction info message
- absolute Paths tried to be converted to relative paths relative to workspace
root to allow simply copy & paste
- Also fixes a inconsistency for checking license existence in x-pack plugin core build
@breskeby breskeby force-pushed the consider-license-sysprop-in-repro-printer branch from 85ce4ce to 70a72aa Compare December 11, 2020 19:14
@breskeby breskeby requested a review from rjernst December 11, 2020 19:47
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM

@breskeby breskeby merged commit 2b2b8a3 into elastic:master Dec 12, 2020
elasticsearchmachine pushed a commit that referenced this pull request Aug 17, 2022
- When a -Dlicense.key sys property is passed to the build we want to consider
this in the test reproduction info message
- Absolute Paths tried to be converted to relative paths relative to workspace
root to allow simply copy & paste
- Also fixes a inconsistency for checking license existence in x-pack plugin core build
mushao999 added a commit to mushao999/elasticsearch that referenced this pull request Oct 23, 2024
- When a -Dlicense.key sys property is passed to the build we want to consider
this in the test reproduction info message
- Absolute Paths tried to be converted to relative paths relative to workspace
root to allow simply copy & paste
- Also fixes a inconsistency for checking license existence in x-pack plugin core build
mushao999 added a commit to mushao999/elasticsearch that referenced this pull request Oct 28, 2024
- When a -Dlicense.key sys property is passed to the build we want to consider
this in the test reproduction info message
- Absolute Paths tried to be converted to relative paths relative to workspace
root to allow simply copy & paste
- Also fixes a inconsistency for checking license existence in x-pack plugin core build
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team v7.17.6 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants