Skip to content

Allow plugin reader to ignore dependents of plugins #12854

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

Closed
nineinchnick wants to merge 3 commits into from
Closed

Allow plugin reader to ignore dependents of plugins #12854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1521143
Allow plugin reader to ignore dependents of plugins
Jun 15, 2022
60fdd2f
Don't exclude server modules when checking impacted modules
Oct 13, 2022
e97dc51
Use correct variable for event name in CI
Oct 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ jobs:
- name: Maven validate
run: |
export MAVEN_OPTS="${MAVEN_INSTALL_OPTS}"
$RETRY $MAVEN validate ${MAVEN_FAST_INSTALL} ${MAVEN_GIB} -Dgib.logImpactedTo=gib-impacted.log -P disable-check-spi-dependencies -pl '!:trino-docs,!:trino-server,!:trino-server-rpm'
$RETRY $MAVEN validate ${MAVEN_FAST_INSTALL} ${MAVEN_GIB} -Dgib.logImpactedTo=gib-impacted.log -P disable-check-spi-dependencies -pl '!:trino-docs'
- name: Set matrix
id: set-matrix
run: |
Expand Down Expand Up @@ -658,10 +658,10 @@ jobs:
- name: Map impacted plugins to features
run: |
export MAVEN_OPTS="${MAVEN_INSTALL_OPTS}"
$MAVEN validate ${MAVEN_FAST_INSTALL} ${MAVEN_GIB} -Dgib.logImpactedTo=gib-impacted.log -pl '!:trino-docs,!:trino-server-rpm'
$MAVEN validate ${MAVEN_FAST_INSTALL} ${MAVEN_GIB} -Dgib.logImpactedTo=gib-impacted.log -pl '!:trino-docs'
Copy link
Copy Markdown
Member

@hashhar hashhar Oct 13, 2022

Choose a reason for hiding this comment

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

the list list in commit message for "Don't exclude server modules when checking impacted modules"

Copy link
Copy Markdown
Member Author

@nineinchnick nineinchnick Oct 14, 2022

Choose a reason for hiding this comment

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

I extracted this commit into #14632

# GIB doesn't run on master, so make sure the file always exist
touch gib-impacted.log
testing/trino-plugin-reader/target/trino-plugin-reader-*-executable.jar -i gib-impacted.log -p core/trino-server/target/trino-server-*-hardlinks/plugin > impacted-features.log
testing/trino-plugin-reader/target/trino-plugin-reader-*-executable.jar -i gib-impacted.log -p core/trino-server/target/trino-server-*-hardlinks/plugin -a core/trino-server,core/trino-server-rpm > impacted-features.log
echo "Impacted plugin features:"
cat impacted-features.log
- name: Product tests artifact
Expand Down Expand Up @@ -766,7 +766,7 @@ jobs:
EOF
- name: Build PT matrix (all)
if: |
github.event.name != 'pull_request' ||
github.event_name != 'pull_request' ||
Copy link
Copy Markdown
Member

@hashhar hashhar Oct 13, 2022

Choose a reason for hiding this comment

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

this was extracted - remove this commit?

steps.filter.outputs.product-tests == 'true' ||
contains(github.event.pull_request.labels.*.name, 'tests:all') ||
contains(github.event.pull_request.labels.*.name, 'tests:all-product')
Expand All @@ -775,7 +775,7 @@ jobs:
./.github/bin/build-pt-matrix-from-impacted-connectors.py -v -m .github/test-pt-matrix.yaml -o matrix.json
- name: Build PT matrix (impacted-features)
if: |
github.event.name == 'pull_request' &&
github.event_name == 'pull_request' &&
steps.filter.outputs.product-tests == 'false' &&
!contains(github.event.pull_request.labels.*.name, 'tests:all') &&
!contains(github.event.pull_request.labels.*.name, 'product-tests:all')
Expand Down
7 changes: 6 additions & 1 deletion testing/trino-plugin-reader/src/main/java/io/trino/server/PluginReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ public class PluginReader
@Option(names = {"-r", "--root-pom"}, description = "Trino root module pom.xml")
private File rootPom = new File("pom.xml");

@Option(names = {"-a", "--allowed-non-plugins"}, split = ",", description = "Allowed non-plugin modules, before ignoring whole impacted modules list; usually dependents of plugins")
private List<String> allowedNonPlugins = List.of();

public static void main(String... args)
{
int exitCode = new CommandLine(new PluginReader()).execute(args);
Expand All @@ -107,7 +110,9 @@ public Integer call()
.collect(toMap(plugin -> plugin.getClass().getName(), identity()));
Stream<Map.Entry<String, String>> modulesStream = requireNonNull(modulesToPlugins).entrySet().stream();
if (impactedModules.isPresent()) {
List<String> nonPluginModules = impactedModules.get().stream().filter(module -> !modulesToPlugins.containsKey(module)).collect(Collectors.toList());
List<String> nonPluginModules = impactedModules.get().stream()
Copy link
Copy Markdown
Member

@hashhar hashhar Oct 13, 2022

Choose a reason for hiding this comment

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

Probably best to rename this variable since it's no longer just nonPluginModules.

Copy link
Copy Markdown
Member

@hashhar hashhar Oct 13, 2022

Choose a reason for hiding this comment

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

Also add context to commit message of " Allow plugin reader to ignore dependents of plugins "

IIUC this means we now apply impact analysis in product tests if change is in core/trino-server or core/trino-server-rpm whereas earlier we'd have just run all tests.

Copy link
Copy Markdown
Member

@hashhar hashhar Oct 13, 2022

Choose a reason for hiding this comment

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

That brings up concern - what happens when in future some plugin module gets used as dependency in other plugin module?

It might not be as concerning since the only impact would be we'd run more product tests than needed?

.filter(module -> !modulesToPlugins.containsKey(module) && !allowedNonPlugins.contains(module))
.collect(Collectors.toList());
if (nonPluginModules.size() != 0) {
log.warn("Impacted modules list includes non-plugin modules, ignoring it: %s", nonPluginModules);
}
Expand Down