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

RFC: task: prefetch: Condense the RHSM entitlement RPM repositories logic to a single step #1850

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eskultety
Copy link
Contributor

Depends on: containerbuildsystem/cachi2#792

Opening as an RFC draft since this solution currently relies on a custom-built cachi2 image which the task was tested with in a private Konflux deployment. Once the dependent PR is merged AND this solution is accepted I will lift the Draft status. Pasting a (publishable) snippet from the pipelines log:

prefetch
=======
Registering with Red Hat subscription manager.
The system has been registered with ID: cc44f731-1d42-44f2-9993-56454f63faa4
The registered system name is: konflux-test-repo-on-pull-rdc2a0834b9fb80d8cdb850c8f3586779-pod
2025-01-22 17:02:50,177 INFO Fetching the gomod dependencies at subpath hello
2025-01-22 17:02:50,186 INFO go.mod reported versions: '1.20'[go], '-'[toolchain]
2025-01-22 17:02:50,187 INFO Using Go release: go1.20
2025-01-22 17:02:50,195 INFO Downloading the gomod dependencies
2025-01-22 17:02:52,891 INFO Retrieving the list of packages
2025-01-22 17:02:53,195 INFO Reading RPM lockfile: /var/workdir/source/rpms.lock.yaml
2025-01-22 17:02:53,197 INFO Downloading files for 'x86_64' architecture.
2025-01-22 17:02:53,203 INFO Using client certificate auth.
2025-01-22 17:02:58,373 INFO All dependencies fetched successfully \o/
2025-01-22 17:02:59,454 INFO Creating repository metadata for repoid 'appstream': /var/workdir/cachi2/output/deps/rpm/x86_64/appstream
2025-01-22 17:02:59,520 INFO Creating /var/workdir/cachi2/output/deps/rpm/x86_64/repos.d/cachi2.repo
Unregistering from: subscription.rhsm.redhat.com:443/subscription
System has been unregistered.

build
====
...
[2/2] STEP 7/8: CMD ["/usr/local/bin/hello"]
[2/2] STEP 8/8: LABEL "build-date"="2025-01-22T17:03:57" "architecture"="x86_64" "vcs-type"="git" "vcs-ref"="a0c881286921d0ae047b932d3dc1a7645394c122" "quay.expires-after"="5d"
[2/2] COMMIT quay.io/<path>/konflux-test-repo:on-pr-a0c881286921d0ae047b932d3dc1a7645394c122
--> 60f0fc490100
Successfully tagged quay.io/<path>/konflux-test-repo:on-pr-a0c881286921d0ae047b932d3dc1a7645394c122
60f0fc49010053f7915bd33d1c646d2015832fc51161576a87ea33ce5851e9ba

This is a partial revert of commit 5f62fe3 which introduced the RH
subscription manager handling logic in form of multiple steps. None of
these steps were ever re-used as a step action. Moreover, we have faced
several issues with RHSM unregister issues because the unregistering
happened in a different context (because it was a separate step).

This patch condenses the original logic from several steps into a
single one - the main prefetch step - and rewrites everything in Bash
to make the task consistent and unified (original used inline Python
for a specific task which may have been arguably been more readable
than 'jq' magic).

Signed-off-by: Erik Skultety <[email protected]>
@eskultety
Copy link
Contributor Author

Worth noting that although early failure isn't currently enforced in Bash, the solution as proposed should still employ something like trap 'subscription-manager unregister' EXIT.

@@ -325,6 +183,50 @@ spec:
script: |
#!/bin/bash

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use this opt set -euo pipefail to make sure that we fail early, and then use trap to unregister? It's confusing to users to see script continues when there was a failures. It generates misleading output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was proposed in a different PR, but sure, I can enable it in a separate commit and as I mentioned in a different response to the PR add the trap.

Comment on lines +280 to +281
creds="$(cat /activation-key/org /activation-key/activationkey)"
IFS=$'\n' read -r RHSM_ORG RHSM_ACT_KEY <<<"$creds"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These 2 lines should not be here, that's some (clearly) failed rebase attempt, consider them deleted in favour of the 2 lines above them.

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