Skip to content

Conversation

@mtojek
Copy link
Contributor

@mtojek mtojek commented Sep 6, 2022

Issue: elastic/package-registry#770

This PR enables the proxy mode feature in the elastic-package stack.

@mtojek mtojek requested a review from a team September 6, 2022 09:54
@mtojek mtojek self-assigned this Sep 6, 2022
@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 6, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-09-07T07:58:46.973+0000

  • Duration: 31 min 1 sec

Test stats 🧪

Test Results
Failed 0
Passed 783
Skipped 0
Total 783

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link
Collaborator

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (32/32) 💚
Files 66.949% (79/118) 👍
Classes 62.424% (103/165) 👍
Methods 49.625% (331/667) 👍
Lines 33.289% (2985/8967) 👎 -0.007
Conditionals 100.0% (0/0) 💚

@mtojek mtojek marked this pull request as ready for review September 6, 2022 10:21
COPY profiles/${PROFILE}/stack/package-registry.config.yml /package-registry/config.yml
COPY stack/development/ /packages/development

ENTRYPOINT ["./package-registry", "--feature-proxy-mode", "--log-level", "debug"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of the features of package-registry have been configured in the docker-compose.yml using environment variables. What about moving settings related to logging to docker-compose file and keep the entrypoint as:

ENTRYPOINT ["./package-registry", "--feature-proxy-mode"]

Since proxy mode is going to be the default behaviour of the package-registry, it would make sense to keep it in the Dockerfile.

And then, moving the log-level parameter to the docker-compose (EPR_LOG_LEVEL?). In the docker-compose-stack.yaml should be something like:

    environment:
      - "EPR_LOG_LEVEL=debug"
      - "EPR_ADDRESS=0.0.0.0:8080"
      - "EPR_METRICS_ADDRESS=0.0.0.0:9000"
      - "EPR_TLS_KEY=/etc/ssl/package-registry/key.pem"
      - "EPR_TLS_CERT=/etc/ssl/package-registry/cert.pem"

I think if the parameter is set in the command line has preference over the environment variables. Moving the log level as environment variable could be changed by developers in the docker-compose without re-building the image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, fixed!

COPY profiles/${PROFILE}/stack/package-registry.config.yml /package-registry/config.yml
COPY stack/development/ /packages/development

ENTRYPOINT ["./package-registry", "--feature-proxy-mode", "--log-level", "debug"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Just one dash ?

Suggested change
ENTRYPOINT ["./package-registry", "--feature-proxy-mode", "--log-level", "debug"]
ENTRYPOINT ["./package-registry", "-feature-proxy-mode", "-log-level", "debug"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@mtojek
Copy link
Contributor Author

mtojek commented Sep 6, 2022

/test

1 similar comment
@mtojek
Copy link
Contributor Author

mtojek commented Sep 6, 2022

/test

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.

👍

COPY profiles/${PROFILE}/stack/package-registry.config.yml /package-registry/config.yml
COPY stack/development/ /packages/development

ENTRYPOINT ["./package-registry", "-feature-proxy-mode"]
Copy link
Member

Choose a reason for hiding this comment

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

It should be also possible to use environment variables to configure this: EPR_FEATURE_PROXY_MODE=true. This may be better than setting the entrypoint, as we are already using environment variables to disable validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

- /packages/development
- /packages/production
- /packages/staging
- /packages/snapshot
Copy link
Member

Choose a reason for hiding this comment

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

🔥 🔥 🔥

// PackageRegistryBaseImage is the base Docker image of the Elastic Package Registry.
const PackageRegistryBaseImage = "docker.elastic.co/package-registry/distribution:snapshot"
// commit SHA of enabled proxy mode (technical preview feature)
const PackageRegistryBaseImage = "docker.elastic.co/package-registry/package-registry:14455fcb7e415d6b4aed48e00d2289718f056162"
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to wait to have a released version with 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.

Yes, as I'm afraid that we may need to add more fixes, and going with the standard release will slow it down. I will replace it with a proper release after some time.

@mtojek mtojek requested review from jsoriano and mrodm September 7, 2022 08:07
@mtojek mtojek merged commit 35cfe28 into elastic:main Sep 7, 2022
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.

4 participants