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

Return just the latest version of the packages if all=true is not set when proxy mode is enabled #1055

Merged
merged 4 commits into from
Jul 13, 2023

Conversation

mrodm
Copy link
Contributor

@mrodm mrodm commented Jul 13, 2023

When using proxy mode, currently if all=true is set just the latest packages are returned.

In this PR, it is updated search endpoint to return all packages in case all=true is not set. It joins the packages returned locally plus the ones returned by the proxy EPR.

In case all=true is set, it will return just the latest packages resulting from merging the packages found locally plues the ones returned by the proxy EPR.

Examples:

  • Running without any local package
 $ curl -s "http://localhost:8080/search?all=true&prerelease=true" | jq -r '.[]|"\(.name) - \(.version)"' | wc -l
4542
 $ curl -s "https://epr.elastic.co/search?all=true&prerelease=true" | jq -r '.[]|"\(.name) - \(.version)"' | wc -l
4542

 $ curl -s "http://localhost:8080/search?prerelease=true" | jq -r '.[]|"\(.name) - \(.version)"' | wc -l
240
12:38:21 mariorodriguez@atlantis:~/Coding/work/integrations/packages/elastic_package_registry @ main - 27f40f6c9/256e01144 * (git) 
 $ curl -s "https://epr.elastic.co/search?prerelease=true" | jq -r '.[]|"\(.name) - \(.version)"' | wc -l
240

 $ curl -s "http://localhost:8080/search?all=true&package=elastic_package_registry&prerelease=true" | jq -r '.[]|"\(.name) - \(.version)"' | wc -l
7
 $ curl -s "https://epr.elastic.co/search?all=true&package=elastic_package_registry&prerelease=true" | jq -r '.[]|"\(.name) - \(.version)"' | wc -l
7
  • Running with 2 new local elastic_package_registry packages:
 $ curl -s "http://localhost:8080/search?all=true&prerelease=true" | jq -r '.[]|"\(.name) - \(.version)"' | wc -l
4544

 $ curl -s "https://epr.elastic.co/search?all=true&prerelease=true" | jq -r '.[]|"\(.name) - \(.version)"' | wc -l
4542

 $ curl -s "http://localhost:8080/search?all=true&package=elastic_package_registry&prerelease=true" | jq -r '.[]|"\(.name) - \(.version)"' | wc -l
9

 $ curl -s "https://epr.elastic.co/search?all=true&package=elastic_package_registry&prerelease=true" | jq -r '.[]|"\(.name) - \(.version)"' | wc -l
7

How to test

  1. Build a package-registry docker image from this branch:
docker build -t docker.elastic.co/package-registry/package-registry:v1.21.0-rc1 .
  1. To test without local packages:
    1. Run locally Elastic Package Registry configured with the proxy remote:
      docker run --rm -p 8080:8080 -e EPR_LOG_LEVEL=debug -e EPR_FEATURE_PROXY_MODE=true -e EPR_PROXY_TO=https://epr.elastic.co -it docker.elastic.co/package-registry/package-registry:v1.21.0-rc1
  2. To test with local packages:
    1. Build some local packages from the integrations repository:
      1. Update manifest and changelog from a package
      2. Execute elastic-package build
      3. Folder where packages have been built needs to be used in the following command. Example: /home/mariorodriguez/Coding/work/integrations/build/packages/
    2. Run locally Elastic Package Registry configured with the proxy remote:
      docker run --rm -p 8080:8080 -e EPR_LOG_LEVEL=debug -e EPR_FEATURE_PROXY_MODE=true -e EPR_PROXY_TO=https://epr.elastic.co  -v /home/mariorodriguez/Coding/work/integrations/build/packages/:/packages/package-registry -it docker.elastic.co/package-registry/package-registry:v1.21.0-rc1

To test if local EPR returns the same exact packages, this script can be run:

#!/bin/bash

HOSTS="http://localhost:8080 https://epr.elastic.co"

QUERIES="all=true all=true&prerelease=true prerelease=true experimental=true"

for url in ${HOSTS}; do
    for query in ${QUERIES}; do
         echo "${url} ${query}"
         curl -s "${url}/search?${query}" | wc -l
    done
done

Relates #873

@mrodm mrodm self-assigned this Jul 13, 2023
@mrodm mrodm requested a review from a team July 13, 2023 10:39
@mrodm mrodm marked this pull request as ready for review July 13, 2023 10:39
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Good catch!

@@ -55,7 +55,9 @@ func searchHandlerWithProxyMode(logger *zap.Logger, indexer Indexer, proxyMode *
return
}
packages = packages.Join(proxiedPackages)
packages = latestPackagesVersion(packages)
if !opts.Filter.AllVersions {
packages = latestPackagesVersion(packages)
Copy link
Member

Choose a reason for hiding this comment

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

Do we have tests covered by this? I would be interested on seeing that we don't duplicate packages, but packages.Join should take care of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were no tests for this proxy mode in search endpoint.

I've added a test here 54d32e7 using proxy mode using all=true and not using it.

@mrodm mrodm requested a review from jsoriano July 13, 2023 13:31
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

👍

@mrodm mrodm changed the title Return all packages if all=true is not set Return all packages if all=true is not set when proxy mode is enabled Jul 13, 2023
@mrodm mrodm changed the title Return all packages if all=true is not set when proxy mode is enabled Return just the latest version of the packages if all=true is not set when proxy mode is enabled Jul 13, 2023
@mrodm mrodm merged commit eaa62bf into elastic:main Jul 13, 2023
1 check passed
@mrodm mrodm deleted the return_all_packages_proxy_search branch July 13, 2023 14: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.

2 participants