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

[BUG] Project integration tests require same version of plugins to be checked in #123

Closed
Tracked by #1375 ...
dblock opened this issue Aug 18, 2021 · 17 comments
Closed
Tracked by #1375 ...
Labels
bug Something isn't working enhancement New request infra Changes to infrastructure, testing, CI/CD, pipelines, etc.

Comments

@dblock
Copy link
Member

dblock commented Aug 18, 2021

Describe the bug

Currently the following files are checked in.

src/test/resources/index-management/opendistro-index-management-1.13.0.0.zip
src/test/resources/notifications/opensearch-notifications-1.1.0.0-SNAPSHOT.zip
src/test/resources/job-scheduler/opensearch-job-scheduler-1.1.0.0-SNAPSHOT.zip

These are moving targets. Furthermore they are of the same release, while one would want to run integration tests against the released versions of any dependency. Which causes a catch-22 problem.

Expected behavior

These files should be deleted. One solution would be to download them during build. Another would be to build them (#119 tried to do that).

Additional context

Consider the developer experience that wants to check out this repo and just run ./gradlew integTest successfully.

@downsrob
Copy link
Contributor

downsrob commented Mar 10, 2022

In the 1.3 branch we currently are still using the job scheduler resource in testing https://github.com/opensearch-project/index-management/tree/1.3/src/test/resources/job-scheduler

@ylwu-amzn
Copy link

ylwu-amzn commented Mar 10, 2022

AD using RCF jar and Protostuff jar https://github.com/opensearch-project/anomaly-detection/tree/1.3/lib, @dblock , we are going to revert the change to remove protostuff.

For RCF jar, we don't have latest version on Maven yet. @wnbts, do you know who is working on publishing RCF 2.x to Maven?

And we have job scheduler zip https://github.com/opensearch-project/anomaly-detection/tree/1.3/src/test/resources/job-scheduler, and BWC test https://github.com/opensearch-project/anomaly-detection/tree/1.3/src/test/resources/org/opensearch/ad/bwc/job-scheduler

@ylwu-amzn
Copy link

Same RCF issue for ml-commons, we have local RCF jar file https://github.com/opensearch-project/ml-commons/tree/1.3/ml-algorithms/lib. Will pull from Maven once 2.x published

@dblock
Copy link
Member Author

dblock commented Mar 11, 2022

@ylwu-amzn Same problem we're checking in the JARs into AD, opensearch-project/anomaly-detection#433

@dblock dblock removed the Beta label Apr 15, 2022
@prudhvigodithi
Copy link
Contributor

prudhvigodithi commented May 11, 2022

Hey @downsrob, @ylwu-amzn, @dblock we have now, a mechanism to publish the zips to maven repo using opensearch.pluginzip, we can delete the zips from checking in to source repo (unless i'm missing something) and can be dowloaded during runtime using the right maven coordinates once they are published.

@ylwu-amzn
Copy link

Thanks @prudhvigodithi , I think @amitgalitz has removed local zip file from AD, check opensearch-project/anomaly-detection#487 and opensearch-project/anomaly-detection#505. We are using latest build link and cloudfront download link, check https://github.com/opensearch-project/anomaly-detection/blob/main/build.gradle#L37

        job_scheduler_build_download = 'https://ci.opensearch.org/ci/dbc/distribution-build-opensearch/' + opensearch_no_snapshot +
                '/latest/linux/x64/tar/builds/opensearch/plugins/opensearch-job-scheduler-' + plugin_no_snapshot + '.zip'
...

        bwcOpenDistroJSDownload = 'https://d3g5vo6xdbdb9a.cloudfront.net/downloads/elasticsearch-plugins/' +
                'opendistro-job-scheduler/opendistro-job-scheduler-1.13.0.0.zip'

@prudhvigodithi should we move to maven? Any benefit?

@prudhvigodithi
Copy link
Contributor

prudhvigodithi commented May 11, 2022

Hey @ylwu-amzn yes pulling zips from maven is the right way to go, as you dont need to manage any custom code moving forward, as using this opensearch.pluginzip the zips will be published to maven repo and can be fetched with right artifcatID, groupID and version.
Example as
mvn org.apache.maven.plugins:maven-dependency-plugin:2.1:get -DrepoUrl=https://aws.oss.sonatype.org/content/repositories/snapshots/ -Dartifact=org.opensearch.plugin:opensearch-job-scheduler:1.3.0.0-SNAPSHOT:zip
@bbarani

@downsrob
Copy link
Contributor

Index Management has replaced checked-in ZIPs with dynamic dependencies as per #305. Now, this issue is not specifically related to that behavior, but should be used to track the progress of getting integration tests to run against dependencies of different versions

@prudhvigodithi
Copy link
Contributor

prudhvigodithi commented May 13, 2022

Hey @downsrob, looks to me like its using the latest URL and some parse logic, using opensearch.pluginzip, you dont need to have all this custom logic from the PR, I would suggest to go with opensearch.pluginzip.
@dblock @bbarani

@prudhvigodithi
Copy link
Contributor

Hey @downsrob and @ylwu-amzn, is it worth to now explore the pulling the zips from maven and removing the checking in files from src/test/resources/?
@dblock @bbarani

@downsrob
Copy link
Contributor

@prudhvigodithi We have already converted to pulling the zips from maven and do not have any checked in zips in src/test/resources. That was tracked in this issue #348 I believe.
My understanding is that this issue is used for tracking the progress of allowing integration tests to run against dependencies of different versions, which is why it is still open despite no longer using checked in zips

@prudhvigodithi
Copy link
Contributor

Got it thanks for the update @downsrob.

@prudhvigodithi
Copy link
Contributor

Hey Starting with release 2.1.0, we now have plugin zips in maven repo
We should be good to remove the checked in zips from repo from 2.x branches.
opensearch-project/opensearch-build#1916 (comment)
Thank you

@downsrob
Copy link
Contributor

downsrob commented Jul 8, 2022

@prudhvigodithi we do not have any checked in zips in the 2.x branches

@prudhvigodithi
Copy link
Contributor

prudhvigodithi commented Jul 8, 2022

Thanks @downsrob,
So can we expect to close this issue once PR: #403 merged.?
Thank you

@bbarani
Copy link
Member

bbarani commented Jul 11, 2022

@downsrob Can we close this issue as we have merged since #403 has been merged now?

@prudhvigodithi
Copy link
Contributor

Hey @downsrob just checking back again, if we can close this issue.
Thank you
@bbarani @CEHENKLE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New request infra Changes to infrastructure, testing, CI/CD, pipelines, etc.
Projects
None yet
Development

No branches or pull requests

6 participants