Skip to content

zookeeper: remove config in root / directory#78639

Open
mbrancato wants to merge 2 commits intowolfi-dev:mainfrom
mbrancato:zk_conf_folder
Open

zookeeper: remove config in root / directory#78639
mbrancato wants to merge 2 commits intowolfi-dev:mainfrom
mbrancato:zk_conf_folder

Conversation

@mbrancato
Copy link

Fixes:
A bug where newer zk packages from wolfi have a /conf folder which should be in /usr/share/java/zookeeper/conf as older packages were. Creating new directories in root is not a standard practice.

Related:

commit for blame:
b6871dc

Pre-review Checklist

For new package PRs only

  • This PR is marked as fixing a pre-existing package request bug
    • Alternatively, the PR is marked as related to a pre-existing package request bug, such as a dependency
  • REQUIRED - The package is available under an OSI-approved or FSF-approved license
  • REQUIRED - The version of the package is still receiving security updates
  • This PR links to the upstream project's support policy (e.g. endoflife.date)

For new version streams

  • The upstream project actually supports multiple concurrent versions.
  • Any subpackages include the version string in their package name (e.g. name: ${{package.name}}-compat)
  • The package (and subpackages) provides: logical unversioned forms of the package (e.g. nodejs, nodejs-lts)
  • If non-streamed package names no longer built, open PR to withdraw them (see WITHDRAWING PACKAGES)

For package updates (renames) in the base images

When updating packages part of base images (i.e. cgr.dev/chainguard/wolfi-base or ghcr.io/wolfi-dev/sdk)

  • REQUIRED cgr.dev/chainguard/wolfi-base and ghcr.io/wolfi-dev/sdk images successfully build
  • REQUIRED cgr.dev/chainguard/wolfi-base and ghcr.io/wolfi-dev/sdk contain no obsolete (no longer built) packages
  • Upon launch, does apk upgrade --latest successfully upgrades packages or performs no actions

For security-related PRs

  • The security fix is recorded in the advisories repo

CVE Scanning: This PR will fail if ANY CVEs are found (fail-any mode). To customize:

  • Must-fix specific CVEs only: Add <!--ci-cve-scan:must-fix: CVE-ID--> markers and remove the line below
  • Fail on any CVEs (default): Keep the marker below
<!--ci-cve-scan:fail-any-->

For version bump PRs

  • The epoch field is reset to 0

For PRs that add patches

  • Patch source is documented

Signed-off-by: Mike Brancato <mbrancato@users.noreply.github.com>
Signed-off-by: Mike Brancato <mbrancato@users.noreply.github.com>
@AnmolVirdi
Copy link
Member

Hi @mbrancato, thank you for taking the time to raise this PR.
Just want to clarify that the change in question is intentional fix rather than a bug. We aligned the packaging with how upstream distributes and bundles zookeeper, where the conf directory resides at the root (/). For reference, the upstream image layout looks like this:

#/apache-zookeeper-3.8.6-bin# ls
bin  conf  docs  lib  LICENSE.txt  NOTICE.txt  README.md  README_packaging.md
#/apache-zookeeper-3.8.6-bin# ls conf/
#/apache-zookeeper-3.8.6-bin# ls ../conf/
configuration.xsl  logback.xml  zoo_sample.cfg

Following the upstream structure makes it easier for users to treat our zookeeper package as a drop-in replacement, which was the main goal behind this adjustment.

That said, I’m happy to discuss further if you think there are edge cases we should consider. Thanks again for the thoughtful contribution.

@mbrancato
Copy link
Author

@AnmolVirdi just do that in your apko config using the paths directive to symlink from /conf to the installed location for zookeeper. The package today still looks nothing like the "official" container which has pretty much all the folders in root. But still, that is a container issue (apko) not a package issue. Only the /conf folder was moved.

@AnmolVirdi
Copy link
Member

@mbrancato couple of pointers:

  1. We do have package configuration that matches the "official" upstream. The zookeeper-compat sub-package in the same manifest takes care of it.
  2. Is there any specific use-case which was blocked or negatively impacted by this change? If there is, I’d really appreciate understanding it better so we can address it appropriately. Else, we should keep it at par with upstream.

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