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

Use default build script for reportsDashboards, observabilityDashboards, queryWorkbenchDashboards and remove customized build script #3055

Merged
merged 5 commits into from
Jan 9, 2023

Conversation

rupal-bq
Copy link
Contributor

@rupal-bq rupal-bq commented Jan 6, 2023

Signed-off-by: Rupal Mahajan [email protected]

Description

  • removed customized build script for reportsDashboards
  • removed customized build script for observabilityDashboards
  • removed customized build script for queryWorkbenchDashboards

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

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.

@rupal-bq rupal-bq requested a review from a team as a code owner January 6, 2023 23:22
@gaiksaya
Copy link
Member

gaiksaya commented Jan 6, 2023

Hi @rupal-bq What do you think about moving this build.sh in the respective repository? Its will be easier to manage too.
cc: @peterzhuamazon @dblock

Signed-off-by: Rupal Mahajan <[email protected]>
@rupal-bq
Copy link
Contributor Author

rupal-bq commented Jan 6, 2023

Hi @rupal-bq What do you think about moving this build.sh in the respective repository? Its will be easier to manage too. cc: @peterzhuamazon @dblock

Seems good idea to me if many plugins have special use cases to handle. But I am not sure what is required to do so.

@gaiksaya
Copy link
Member

gaiksaya commented Jan 6, 2023

Just move the script under scripts folder. Example https://github.com/opensearch-project/alerting/tree/main/scripts
and delete from here

@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2023

Codecov Report

Merging #3055 (0cb8109) into main (af1be01) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #3055   +/-   ##
=======================================
  Coverage   93.17%   93.17%           
=======================================
  Files         167      167           
  Lines        4602     4602           
=======================================
  Hits         4288     4288           
  Misses        314      314           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@rupal-bq
Copy link
Contributor Author

rupal-bq commented Jan 9, 2023

Just move the script under scripts folder. Example https://github.com/opensearch-project/alerting/tree/main/scripts and delete from here

Deleted the script from here. As the chromium is removed and the repo split is done, I think dashboards-reporting can now use the default script.

@gaiksaya gaiksaya changed the title Remove chromium binary in dashboards-reports artifact Use default build script for reportsDashboards and remove customized build script Jan 9, 2023
@gaiksaya
Copy link
Member

gaiksaya commented Jan 9, 2023

@zelinh You okay with this change?

Copy link
Member

@zelinh zelinh left a comment

Choose a reason for hiding this comment

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

LGTM.

@vmmusings
Copy link
Member

@rupal-bq @zelinh Does removing the script will automatically trigger default build script. We want to remove for observability Dashboards as well?

@zelinh
Copy link
Member

zelinh commented Jan 9, 2023

@rupal-bq @zelinh Does removing the script will automatically trigger default build script. We want to remove for observability Dashboards as well?

@vamsi-amazon Our current workflow actually have a list for place to look for the build script.

def find_build_script(cls, project: str, component_name: str, git_dir: str) -> str:
paths = [
os.path.realpath(os.path.join(cls.component_scripts_path, component_name, "build.sh")),
os.path.realpath(os.path.join(git_dir, component_name, "build.sh")),
os.path.realpath(os.path.join(git_dir, "build.sh")),
os.path.realpath(os.path.join(git_dir, "scripts", "build.sh")),
os.path.realpath(
os.path.join(
cls.default_scripts_path,
project.replace(" ", "-").lower(),
"build.sh",
)
),
]
return cls.__find_script("build.sh", paths)

If it doesn't find the build script from all of those location above, the last choice is to use the default build script.

@rupal-bq
Copy link
Contributor Author

rupal-bq commented Jan 9, 2023

For dashboards-observability, I don't see a build script at https://github.com/opensearch-project/dashboards-observability.

I will update this PR to remove https://github.com/opensearch-project/opensearch-build/blob/main/scripts/components/observabilityDashboards/build.sh as well so default can be used

@rupal-bq rupal-bq changed the title Use default build script for reportsDashboards and remove customized build script Use default build script for reportsDashboards, observabilityDashboards and remove customized build script Jan 9, 2023
@rupal-bq rupal-bq changed the title Use default build script for reportsDashboards, observabilityDashboards and remove customized build script Use default build script for reportsDashboards, observabilityDashboards, queryWorkbenchDashboards and remove customized build script Jan 9, 2023
@gaiksaya gaiksaya merged commit e354cec into opensearch-project:main Jan 9, 2023
@rupal-bq rupal-bq deleted the main-remove-chromium branch January 31, 2023 04:25
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.

7 participants