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

Update folder names for pre-installed plugins to kebab-case #385

Conversation

VachaShah
Copy link
Contributor

@VachaShah VachaShah commented May 27, 2021

Description

This PR updates the folder names for pre-installed plugins in src/plugins to kebab-case to maintain consistency as described in Issue #322.

Testing

The following commands succeeded:

  • yarn test:jest
  • TEST_OPENSEARCH_FROM=/path/to/extracted/build/artifact yarn test:jest_integration

Issues Resolved

Part 2 of #322.

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

Signed-off-by: Vacha Shah <vachshah@amazon.com>
Signed-off-by: Vacha Shah <vachshah@amazon.com>
Signed-off-by: Vacha Shah <vachshah@amazon.com>
Signed-off-by: Vacha Shah <vachshah@amazon.com>
@VachaShah VachaShah changed the title Update preinstalled plugins to kebab case Update folder names for pre-installed plugins to kebab-case May 27, 2021
@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed 0d05b77

Signed-off-by: Vacha Shah <vachshah@amazon.com>
@opensearch-ci-bot
Copy link
Collaborator

✅   DCO Check Passed d8f7f67

@mihirsoni
Copy link
Contributor

@VachaShah Thanks for the PR, I see 5k files changes, are these expected?

@mihirsoni
Copy link
Contributor

Also, if we decide to go with this approach, we will need to break the the PRs in smaller one, else Git will not be able to carry forward the history. IMO we should do this change as part of 2.0.

@VachaShah
Copy link
Contributor Author

@VachaShah Thanks for the PR, I see 5k files changes, are these expected?

@mihirsoni Yes, there are so many files since the rename is for all the pre-installed plugins folders inside src/plugins.

@VachaShah
Copy link
Contributor Author

Also, if we decide to go with this approach, we will need to break the the PRs in smaller one, else Git will not be able to carry forward the history. IMO we should do this change as part of 2.0.

I tried to break the PR for each with a bunch of plugins in each but there are files which would be common in each of them, making it harder to break down. I thought it would be easier if the rename is just in one PR in terms of commit history, LMK what you think.

@dblock
Copy link
Member

dblock commented May 28, 2021

This is a bunch of renaming and seems harmless enough, LGTM.

@dblock
Copy link
Member

dblock commented May 28, 2021

@mihirsoni I wonder whether it's worth putting this in 1.0 to make it easier for upgrades 1.0 -> 1.x.

@mihirsoni
Copy link
Contributor

mihirsoni commented May 28, 2021

@mihirsoni I wonder whether it's worth putting this in 1.0 to make it easier for upgrades 1.0 -> 1.x.

Hi @dblock this is my concerns
I think we don't need to make it right now in RC we can hit for GA.

  • We need this to be broken down in smaller PR , this is too big PR and can't review throughly and this loses git history.
  • Need to ensure all the integration / functional tests are passing.
  • I don't see harm in doing it gradually.

@VachaShah
Copy link
Contributor Author

@mihirsoni I will work on breaking down this PR into smaller ones, mostly one for each plugin. I have also updated the description for testing as well.

@kavilla
Copy link
Member

kavilla commented Jun 3, 2021

Can this PR be closed?

@VachaShah
Copy link
Contributor Author

Can this PR be closed?

Yes I have created the smaller PRs for all the plugins, closing this PR now.

@VachaShah VachaShah closed this Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants