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.

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

Jump to bottom

[Plugins] update default url for installing plugins #1316

Merged
merged 1 commit into from
Mar 8, 2022
Merged

[Plugins] update default url for installing plugins #1316

merged 1 commit into from
Mar 8, 2022

Conversation

kavilla
Copy link
Member

@kavilla kavilla commented Mar 3, 2022 •
edited
Loading

Description

Update old and invalid reference to custom plugins within
the OpenSearch Project.

Note: At the time that this is committed there is no gurantee
that the latest build is the build that was select as the
final build.

Signed-off-by: Kawika Avilla [email protected]

Issues Resolved

#1038

Check List

  • New functionality includes testing.
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

@kavilla kavilla requested a review from a team as a code owner March 3, 2022 21:31
@kavilla kavilla requested review from ananzh and tmarkley March 4, 2022 00:23
@kavilla kavilla requested a review from tmarkley March 5, 2022 09:37
@kavilla
[Plugins] update default url for installing plugins
d728364 
Update old and invalid reference to custom plugins within
the OpenSearch Project.

Note: At the time that this is committed there is no gurantee
that the latest build is the build that was select as the
final build.

Issue resolved:
opensearch-project#1038

Signed-off-by: Kawika Avilla <[email protected]>
@kavilla kavilla requested a review from ashwin-pc March 7, 2022 22:29
ashwin-pc
ashwin-pc approved these changes Mar 7, 2022
View reviewed changes
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

LGTM!

src/cli_plugin/install/settings.js
return [plugin, generatePluginUrl(version, plugin)];
}

function generatePluginUrl(version, plugin) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This could have just been in generateUrls since that function doesn't do anything anymore 😂

Copy link
Member

Choose a reason for hiding this comment

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

I think there is one called generateUrls (line 42) right?

ananzh
ananzh approved these changes Mar 8, 2022
View reviewed changes
Copy link
Member

@ananzh ananzh left a comment

Choose a reason for hiding this comment

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

LGTM

src/cli_plugin/install/settings.js
return [plugin, generatePluginUrl(version, plugin)];
}

function generatePluginUrl(version, plugin) {
Copy link
Member

Choose a reason for hiding this comment

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

I think there is one called generateUrls (line 42) right?

@kavilla kavilla merged commit 9c82c52 into opensearch-project:main Mar 8, 2022
@kavilla kavilla deleted the avillk/plugin_http branch March 8, 2022 18:55
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 8, 2022
@kavilla @github-actions
[Plugins] update default url for installing plugins (#1316)
c1cf7dd 
Update old and invalid reference to custom plugins within
the OpenSearch Project.

Note: At the time that this is committed there is no gurantee
that the latest build is the build that was select as the
final build.

Issue resolved:
#1038

Signed-off-by: Kawika Avilla <[email protected]>
(cherry picked from commit 9c82c52)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Mar 8, 2022
Merged
ananzh pushed a commit that referenced this pull request Mar 8, 2022
@opensearch-trigger-bot @kavilla
[Plugins] update default url for installing plugins (#1316) (#1328)
7974000 
Update old and invalid reference to custom plugins within
the OpenSearch Project.

Note: At the time that this is committed there is no gurantee
that the latest build is the build that was select as the
final build.

Issue resolved:
#1038

Signed-off-by: Kawika Avilla <[email protected]>
(cherry picked from commit 9c82c52)

Co-authored-by: Kawika Avilla <[email protected]>
tmarkley
tmarkley reviewed Mar 15, 2022
View reviewed changes
src/cli_plugin/install/settings.test.js
Comment on lines +41 to +42
const ORIGINAL_PLATFORM = process.platform;
const ORIGINAL_ARCHITECTURE = process.arch;
Copy link
Contributor

Choose a reason for hiding this comment

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

This causes tests to be inconsistent depending on which platform/architecture you're running them from.

Copy link
Contributor

Choose a reason for hiding this comment

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

My snapshots were updated on my arm64 machine and then they failed in GitHub Actions: https://github.com/opensearch-project/OpenSearch-Dashboards/runs/5548019026?check_suite_focus=true

Copy link
Contributor

Choose a reason for hiding this comment

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

Going to submit a PR to fix this now.

Copy link
Contributor

Choose a reason for hiding this comment

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

#1346

tmarkley pushed a commit to tmarkley/OpenSearch-Dashboards that referenced this pull request Mar 15, 2022
Fixes inconsistent plugin installation tests
680b579 
* The tests assumed that the platform/architecture combo the tests were
  run from was always linux/x64.
* Introduced in opensearch-project#1316.

Signed-off-by: Tommy Markley <[email protected]>
@tmarkley tmarkley mentioned this pull request Mar 15, 2022
Merged
7 tasks
tmarkley pushed a commit to tmarkley/OpenSearch-Dashboards that referenced this pull request Mar 17, 2022
Fixes inconsistent plugin installation tests
0a4fe56 
* The tests assumed that the platform/architecture combo the tests were
  run from was always linux/x64.
* Introduced in opensearch-project#1316.

Signed-off-by: Tommy Markley <[email protected]>
kavilla pushed a commit that referenced this pull request Mar 23, 2022
Fixes inconsistent plugin installation tests (#1346)
7aba935 
* The tests assumed that the platform/architecture combo the tests were
  run from was always linux/x64.
* Introduced in #1316.

Signed-off-by: Tommy Markley <[email protected]>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 23, 2022
@github-actions
Fixes inconsistent plugin installation tests (#1346)
0089deb 
* The tests assumed that the platform/architecture combo the tests were
  run from was always linux/x64.
* Introduced in #1316.

Signed-off-by: Tommy Markley <[email protected]>
(cherry picked from commit 7aba935)
tmarkley pushed a commit that referenced this pull request Mar 23, 2022
@opensearch-trigger-bot
Fixes inconsistent plugin installation tests (#1346) (#1372)
253d74b 
* The tests assumed that the platform/architecture combo the tests were
  run from was always linux/x64.
* Introduced in #1316.

Signed-off-by: Tommy Markley <[email protected]>
(cherry picked from commit 7aba935)
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 28, 2022
@github-actions
Fixes inconsistent plugin installation tests (#1346)
074f615 
* The tests assumed that the platform/architecture combo the tests were
  run from was always linux/x64.
* Introduced in #1316.

Signed-off-by: Tommy Markley <[email protected]>
(cherry picked from commit 7aba935)
ananzh pushed a commit that referenced this pull request Apr 28, 2022
@opensearch-trigger-bot
Fixes inconsistent plugin installation tests (#1346) (#1508)
49ef208 
* The tests assumed that the platform/architecture combo the tests were
  run from was always linux/x64.
* Introduced in #1316.

Signed-off-by: Tommy Markley <[email protected]>
(cherry picked from commit 7aba935)

Co-authored-by: Tommy Markley <[email protected]>
@seraphjiang seraphjiang mentioned this pull request Aug 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmarkley tmarkley tmarkley left review comments

@ashwin-pc ashwin-pc ashwin-pc approved these changes

@ananzh ananzh ananzh approved these changes

Assignees
No one assigned
Labels
v1.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kavilla @tmarkley @ashwin-pc @ananzh