Added logic to allow {dot} files on startup #1437

ryanbogan · 2021-10-25T22:21:36Z

Signed-off-by: Ryan Bogan [email protected]

Description

Fixes a bug that would not allow OpenSearch to run when a {dot} file (i.e. ".test") was present in the plugins folder.

Issues Resolved

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

opensearch-ci-bot · 2021-10-25T22:22:03Z

Can one of the admins verify this patch?

opensearch-ci-bot · 2021-10-25T22:23:15Z

✅ DCO Check Passed 9165c4cbde3910754983d277541484b5cb90fa32

opensearch-ci-bot · 2021-10-25T22:23:59Z

✅ Gradle Wrapper Validation success 9165c4cbde3910754983d277541484b5cb90fa32

opensearch-ci-bot · 2021-10-25T22:32:05Z

✅ Gradle Precommit success 9165c4cbde3910754983d277541484b5cb90fa32

dblock

So, we have findPluginDirs that returns the list of plugin directories. Today it also returns a bunch of files that start with . and this PR attempts to filter it. This is difficult to grok and even more difficult to test.

Instead, consider modifying findPluginDirs to only return the plugin directories we care about, and write a test for it.

We also likely want to warn when we encounter something that's not a plugin in the plugins directory.

opensearch-ci-bot · 2021-10-26T14:58:29Z

✅ Gradle Wrapper Validation success 9165c4cbde3910754983d277541484b5cb90fa32

opensearch-ci-bot · 2021-10-26T15:00:32Z

❌ DCO Check Failed 9165c4cbde3910754983d277541484b5cb90fa32
Run ./dev-tools/signoff-check.sh remotes/origin/main 9165c4cbde3910754983d277541484b5cb90fa32 to check locally
Use git commit with -s to add 'Signed-of-by: {EMAIL}' on impacted commits

opensearch-ci-bot · 2021-10-26T15:19:59Z

✅ Gradle Precommit success 9165c4cbde3910754983d277541484b5cb90fa32

ryanbogan · 2021-10-26T15:24:50Z

I'll modify findPluginDirs to only return the plugins that are directories and return a warning each time a {dot} file is encountered.

nknize · 2021-10-26T17:18:31Z

hi @ryanbogan can you rebase your PR w/ latest main branch? The DCO check wasn't your fault. A dirty commit made it into main and this has since been corrected.

opensearch-ci-bot · 2021-10-27T16:05:00Z

❌ DCO Check Failed 2b1822c4e1859fb2d507e866d97858239cf79a3a
Run ./dev-tools/signoff-check.sh remotes/origin/main 2b1822c4e1859fb2d507e866d97858239cf79a3a to check locally
Use git commit with -s to add 'Signed-of-by: {EMAIL}' on impacted commits

opensearch-ci-bot · 2021-10-27T16:05:01Z

✅ Gradle Wrapper Validation success 2b1822c4e1859fb2d507e866d97858239cf79a3a

ryanbogan · 2021-10-27T16:06:24Z

I rebased but it still failed the DCO check

opensearch-ci-bot · 2021-10-27T16:10:49Z

✅ Gradle Precommit success 2b1822c4e1859fb2d507e866d97858239cf79a3a

opensearch-ci-bot · 2021-10-27T18:27:04Z

✅ DCO Check Passed 885936329d5f438985de395882de1d6df4752114

opensearch-ci-bot · 2021-10-27T18:30:04Z

✅ Gradle Wrapper Validation success 885936329d5f438985de395882de1d6df4752114

opensearch-ci-bot · 2021-10-27T18:33:48Z

✅ Gradle Precommit success 885936329d5f438985de395882de1d6df4752114

VachaShah · 2021-10-27T21:19:36Z

start gradle check

opensearch-ci-bot · 2021-10-27T21:38:54Z

❌ Gradle Check failure 885936329d5f438985de395882de1d6df4752114
Log 832

Reports 832

dblock

This needs tests for all those added scenarios, like the specific file name(s), and logger.warn.

dblock · 2021-10-28T00:19:45Z

server/src/main/java/org/opensearch/plugins/PluginsService.java

+                        || plugin.getFileName().toString().startsWith(".removing-")
+                        || plugin.getFileName().toString().startsWith(".")
+                            && !Files.isDirectory(plugin)
+                            && !plugin.getFileName().toString().equals(".DS_Store")) {


Why do we care about .DS_Store? It starts with . ...

Without that it would fail the unit tests for DesktopServicesStore files

The code is wrong then. Isn't the intent to skip all . files?

I don't think we should skip .DS_Store files in this method because an error needs to be thrown in readPluginBundle() if a .DS_Store is encountered. I'm trying to skip all . files except for those with this code.

Maybe I don't understand what we're trying to accomplish. Why would we ignore .DS_Store? Because it's a well known special file? It's not expected in that folder AFAIK.

I think that was the reason, but if you think it's better to allow all . files, including .DS_Store, on startup I can implement that.

Signed-off-by: Ryan Bogan <[email protected]>

opensearch-ci-bot · 2021-11-02T22:05:57Z

✅ Gradle Wrapper Validation success 974e0b5

opensearch-ci-bot · 2021-11-02T22:10:23Z

✅ Gradle Precommit success 974e0b5

opensearch-ci-bot · 2021-11-02T22:49:06Z

✅ Gradle Check success 974e0b5
Log 960

Reports 960

dblock · 2021-11-04T14:02:40Z

Let's backport this into 1.x, and please check whether we say anything about this in docs - may need to be updated.

* Added logic to allow {dot} files on startup Signed-off-by: Ryan Bogan <[email protected]> * Ensures that only plugin directories are returned by findPluginDirs() Signed-off-by: Ryan Bogan <[email protected]> * Prevents . files from being returned as plugins Signed-off-by: Ryan Bogan <[email protected]>

dblock requested changes Oct 26, 2021

View reviewed changes

nknize force-pushed the main branch from 115c0a4 to f469d53 Compare October 26, 2021 14:54

ryanbogan force-pushed the main branch from 2b1822c to 8859363 Compare October 27, 2021 18:24

ryanbogan requested a review from dblock October 27, 2021 18:37

dblock requested changes Oct 28, 2021

View reviewed changes

ryanbogan added 3 commits October 29, 2021 20:38

Added logic to allow {dot} files on startup

acc10ca

Signed-off-by: Ryan Bogan <[email protected]>

Ensures that only plugin directories are returned by findPluginDirs()

c26b5c0

Signed-off-by: Ryan Bogan <[email protected]>

Prevents . files from being returned as plugins

974e0b5

Signed-off-by: Ryan Bogan <[email protected]>

ryanbogan force-pushed the main branch from 8859363 to 974e0b5 Compare November 2, 2021 22:04

ryanbogan requested a review from dblock November 2, 2021 22:05

dblock approved these changes Nov 4, 2021

View reviewed changes

dblock merged commit 286121f into opensearch-project:main Nov 4, 2021

dblock added the pending backport Identifies an issue or PR that still needs to be backported label Nov 4, 2021

ryanbogan mentioned this pull request Nov 8, 2021

[Backport] Added logic to allow {dot} files on startup (#1437) #1516

Merged

2 tasks

ryanbogan mentioned this pull request Nov 15, 2021

[Backport] Added logic to allow {dot} files on startup (#1437) #1543

Closed

3 tasks

tlfeng added v1.2.0 Issues related to version 1.2.0 v2.0.0 Version 2.0.0 labels Nov 16, 2021

dblock removed the pending backport Identifies an issue or PR that still needs to be backported label Dec 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added logic to allow {dot} files on startup #1437

Added logic to allow {dot} files on startup #1437

ryanbogan commented Oct 25, 2021

opensearch-ci-bot commented Oct 25, 2021

opensearch-ci-bot commented Oct 25, 2021

opensearch-ci-bot commented Oct 25, 2021

opensearch-ci-bot commented Oct 25, 2021

dblock left a comment •

edited

Loading

opensearch-ci-bot commented Oct 26, 2021

opensearch-ci-bot commented Oct 26, 2021

opensearch-ci-bot commented Oct 26, 2021

ryanbogan commented Oct 26, 2021

nknize commented Oct 26, 2021

opensearch-ci-bot commented Oct 27, 2021

opensearch-ci-bot commented Oct 27, 2021

ryanbogan commented Oct 27, 2021

opensearch-ci-bot commented Oct 27, 2021

opensearch-ci-bot commented Oct 27, 2021

opensearch-ci-bot commented Oct 27, 2021

opensearch-ci-bot commented Oct 27, 2021

VachaShah commented Oct 27, 2021

opensearch-ci-bot commented Oct 27, 2021

dblock left a comment •

edited

Loading

dblock Oct 28, 2021

ryanbogan Oct 28, 2021

dblock Oct 28, 2021

ryanbogan Oct 28, 2021 •

edited

Loading

dblock Oct 28, 2021

ryanbogan Oct 28, 2021

opensearch-ci-bot commented Nov 2, 2021

opensearch-ci-bot commented Nov 2, 2021

opensearch-ci-bot commented Nov 2, 2021

dblock commented Nov 4, 2021

Added logic to allow {dot} files on startup #1437

Added logic to allow {dot} files on startup #1437

Conversation

ryanbogan commented Oct 25, 2021

Description

Issues Resolved

Check List

opensearch-ci-bot commented Oct 25, 2021

opensearch-ci-bot commented Oct 25, 2021

opensearch-ci-bot commented Oct 25, 2021

opensearch-ci-bot commented Oct 25, 2021

dblock left a comment • edited Loading

Choose a reason for hiding this comment

opensearch-ci-bot commented Oct 26, 2021

opensearch-ci-bot commented Oct 26, 2021

opensearch-ci-bot commented Oct 26, 2021

ryanbogan commented Oct 26, 2021

nknize commented Oct 26, 2021

opensearch-ci-bot commented Oct 27, 2021

opensearch-ci-bot commented Oct 27, 2021

ryanbogan commented Oct 27, 2021

opensearch-ci-bot commented Oct 27, 2021

opensearch-ci-bot commented Oct 27, 2021

opensearch-ci-bot commented Oct 27, 2021

opensearch-ci-bot commented Oct 27, 2021

VachaShah commented Oct 27, 2021

opensearch-ci-bot commented Oct 27, 2021

dblock left a comment • edited Loading

Choose a reason for hiding this comment

dblock Oct 28, 2021

Choose a reason for hiding this comment

ryanbogan Oct 28, 2021

Choose a reason for hiding this comment

dblock Oct 28, 2021

Choose a reason for hiding this comment

ryanbogan Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

dblock Oct 28, 2021

Choose a reason for hiding this comment

ryanbogan Oct 28, 2021

Choose a reason for hiding this comment

opensearch-ci-bot commented Nov 2, 2021

opensearch-ci-bot commented Nov 2, 2021

opensearch-ci-bot commented Nov 2, 2021

dblock commented Nov 4, 2021

dblock left a comment •

edited

Loading

dblock left a comment •

edited

Loading

ryanbogan Oct 28, 2021 •

edited

Loading