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

Remove job-scheduler build.sh script to favor original repo build #1453

Merged
merged 2 commits into from
Feb 1, 2022

Conversation

amitgalitz
Copy link
Member

Signed-off-by: Amit Galitzky [email protected]

Description

This PR syncs up the build.sh script with what is currently present in job-scheduler based of this recent PR (opensearch-project/job-scheduler#117) in job scheduler plugin. This allows for job-scheduler to be published to maven now along with job-scheduler-spi. Before only spi was historically published.

Issues Resolved

opensearch-project/job-scheduler#114

Check List

  • 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.

@dblock
Copy link
Member

dblock commented Jan 12, 2022

Do we need this script here? If all building branches of job-scheduler have it, delete it altogether from here.

@amitgalitz
Copy link
Member Author

Do we need this script here? If all building branches of job-scheduler have it, delete it altogether from here.

I think the idea is to have it both here and the job-scheduler script will act as a backup. Having them in opensearch-build I assume is an easy way to consistently make sure job-scheduler always gets built. Additionally we haven't yet backported changes to other branches in job-scheduler. @mch2 do you have anything else to add to this.

@dblock
Copy link
Member

dblock commented Jan 12, 2022

Do we need this script here? If all building branches of job-scheduler have it, delete it altogether from here.

I think the idea is to have it both here and the job-scheduler script will act as a backup. Having them in opensearch-build I assume is an easy way to consistently make sure job-scheduler always gets built. Additionally we haven't yet backported changes to other branches in job-scheduler. @mch2 do you have anything else to add to this.

We only did this because it was faster to make changes that applied to multiple versions, the plan has always been to delete these scripts from this repo.

@mch2
Copy link
Member

mch2 commented Jan 13, 2022

Do we need this script here? If all building branches of job-scheduler have it, delete it altogether from here.

I think the idea is to have it both here and the job-scheduler script will act as a backup. Having them in opensearch-build I assume is an easy way to consistently make sure job-scheduler always gets built. Additionally we haven't yet backported changes to other branches in job-scheduler. @mch2 do you have anything else to add to this.

We only did this because it was faster to make changes that applied to multiple versions, the plan has always been to delete these scripts from this repo.

Yeah this was so we could move quickly when setting this up. @amitgalitz Lets take this opportunity to get this backported in supported branches of job-scheduler and then remove it here.

@amitgalitz
Copy link
Member Author

Do we need this script here? If all building branches of job-scheduler have it, delete it altogether from here.

I think the idea is to have it both here and the job-scheduler script will act as a backup. Having them in opensearch-build I assume is an easy way to consistently make sure job-scheduler always gets built. Additionally we haven't yet backported changes to other branches in job-scheduler. @mch2 do you have anything else to add to this.

We only did this because it was faster to make changes that applied to multiple versions, the plan has always been to delete these scripts from this repo.

Yeah this was so we could move quickly when setting this up. @amitgalitz Lets take this opportunity to get this backported in supported branches of job-scheduler and then remove it here.

Sounds good, I'll change this PR to remove the job-scheduler folder from components after it gets back-ported in job-scheduler to the necessary branches.

@abhinavGupta16
Copy link
Contributor

Issue this PR resolved is closed - opensearch-project/job-scheduler#114

Should we reopen the issue or create a new one for this?

@amitgalitz amitgalitz force-pushed the fix-job-scheduler-upload branch from 1d8a474 to 66f7f15 Compare January 20, 2022 16:48
@amitgalitz
Copy link
Member Author

Issue this PR resolved is closed - opensearch-project/job-scheduler#114

Should we reopen the issue or create a new one for this?

Currently the issue should be reopened until we backport the build.sh changes from that PR into all supported branches in job-scheduler. After those are backported. I will make this PR non-draft so we can remove the build.sh script from this repo for job-scheduler.

@codecov-commenter
Copy link

codecov-commenter commented Jan 20, 2022

Codecov Report

Merging #1453 (c11f06e) into main (50e0afe) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #1453   +/-   ##
=========================================
  Coverage     94.46%   94.46%           
  Complexity       12       12           
=========================================
  Files           153      153           
  Lines          3233     3233           
  Branches         23       23           
=========================================
  Hits           3054     3054           
  Misses          172      172           
  Partials          7        7           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50e0afe...c11f06e. Read the comment docs.

@dblock
Copy link
Member

dblock commented Jan 28, 2022

@amitgalitz What are we doing with this?

@amitgalitz
Copy link
Member Author

@amitgalitz What are we doing with this?

This change should go through once build.sh has been updated on all supported job-scheduler branches. I have this PR out that I have put on hold for a few days. I am currently trying to figure out the CI issue. opensearch-project/job-scheduler#120

@amitgalitz amitgalitz changed the title sync job scheduler build.sh script to publish job-scheduler jar Remove job-scheduler build.sh script to favor original repo build Feb 1, 2022
@amitgalitz amitgalitz marked this pull request as ready for review February 1, 2022 20:20
@amitgalitz amitgalitz requested a review from a team as a code owner February 1, 2022 20:20
@peternied
Copy link
Member

@amitgalitz I've restarted the failed CI workflow, that looks like an unrelated issue that has since been resolved.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

I believe that if you rebase the error will get fixed.

@amitgalitz amitgalitz force-pushed the fix-job-scheduler-upload branch from 4e80487 to c11f06e Compare February 1, 2022 21:03
@amitgalitz amitgalitz marked this pull request as draft February 1, 2022 21:37
@peternied peternied self-requested a review February 1, 2022 22:16
@peternied peternied marked this pull request as ready for review February 1, 2022 22:16
@dblock dblock merged commit 4c3ae78 into opensearch-project:main Feb 1, 2022
peterzhuamazon pushed a commit to peterzhuamazon/opensearch-build that referenced this pull request Feb 16, 2022
…ensearch-project#1453)

* removed shadow publication to reguler publish on job scheduler

Signed-off-by: Amit Galitzky <[email protected]>

* removed job-scheduler build.sh script

Signed-off-by: Amit Galitzky <[email protected]>
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.

6 participants