-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Rename system property to change bwc checkout behavior #45574
Rename system property to change bwc checkout behavior #45574
Conversation
Signed-off-by: Mark Vieira <portugee@gmail.com>
Pinging @elastic/es-core-infra |
I've opened https://github.com/elastic/infra/pull/13812 for the corresponding CI changes related to this. |
@elasticmachine run elasticsearch-ci/1 |
@elasticmachine run elasticsearch-ci/2 |
distribution/bwc/build.gradle
Outdated
@@ -104,7 +104,7 @@ bwcVersions.forPreviousUnreleased { BwcVersions.UnreleasedVersionInfo unreleased | |||
dependsOn fetchLatest | |||
doLast { | |||
String refspec = System.getProperty("tests.bwc.refspec.${bwcBranch}", "${remote}/${bwcBranch}") | |||
if (System.getProperty("tests.bwc.checkout.align") != null) { | |||
if (System.getProperty("org.elasticsearch.bwc.checkout.align") != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are other properties also here, like tests.bwc.refspec just above this too. Can we also just remove the tests prefix? For the refspec it is something we occationally need to specify locally, so having an extra long property name makes it both more cumbersome to type and difficult to remember.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm less concerned about that one since it's only used in the exceptional case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do also pass it from CI in the forward comparability tests.
+1 on having consistent parameter names for bwc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've renamed tests.bwc.refspec
to org.elasticsearch.bwc.refspec
. I've updated the associated infra PR as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please remove the o.e prefix? I type this whenever testing bwc changes, so the extra length does matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I've removed the verbose prefix and updated the corresponding infra PR.
Signed-off-by: Mark Vieira <portugee@gmail.com>
Signed-off-by: Mark Vieira <portugee@gmail.com>
Signed-off-by: Mark Vieira <portugee@gmail.com>
@elasticmachine run elasticsearch-ci/2 |
This renames the
tests.bwc.checkout.align
system property toorg.elasticsearch.bwc.checkout.align
. The reason for this is that system properties prefixed withtests
are by convention passed to all test workers. We don't want to do this for two reasons. First, there's no need to pass this on to test workers as it's inapplicable to any specific tests. Second, it then is considered an "input" to all test tasks which means changing this property changes the cache key for all tests. Since this system property doesn't directly affect test execution behavior, there should be no reason to track it as an input.