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

ensure things are properly signed before uploading to stage #425

Merged
merged 5 commits into from
Sep 13, 2024

Conversation

evgeni
Copy link
Member

@evgeni evgeni commented Sep 9, 2024

No description provided.

@evgeni evgeni marked this pull request as ready for review September 9, 2024 08:21
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I debated adding a noop to sign_stage_rpms but I think it's not really in the style of the whole repo so I think your approach is fine.

verify_stage_sigs Outdated Show resolved Hide resolved
verify_stage_sigs Outdated Show resolved Hide resolved
@@ -67,6 +68,7 @@
RPM_PACKAGES=()
PACKAGING_PR=${PACKAGING_PR:-true}
GPG_EXPIRE="1y"
STAGE_LOCAL_BASE="tmp/$PROJECT/$VERSION"

Check warning

Code scanning / shellcheck

STAGE_LOCAL_BASE appears unused. Verify use (or export if used externally). Warning

STAGE_LOCAL_BASE appears unused. Verify use (or export if used externally).
Copy link
Member Author

Choose a reason for hiding this comment

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

we could also use RPMDIR (which is currently unused) here… questions and choices.

@ehelms do you recall why tmp/ was used vs the previously existing path?

Copy link
Member

Choose a reason for hiding this comment

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

I suspect it was previously used by the other script and @ehelms didn't want to conflict with it, but now that koji is cleaned up it's an unused variable. When I introduced it, it made sense to me to store it in the release itself.

Copy link
Member

Choose a reason for hiding this comment

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

@ehelms do you recall why tmp/ was used vs the previously existing path?

What previously existing path? I was the first to introduce the notion of generating staging repositories locally :)

Copy link
Member Author

Choose a reason for hiding this comment

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

But not the first one who had to download rpms, sign them and upload the signatures, which we did in RPMDIR

Copy link
Member

Choose a reason for hiding this comment

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

Fair. My motivation was that I wanted the generation of the repositories to be co-located, easy to clean up and to mimic the structure they would have on the staging repository server to allow easier testing and verification of them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aye. It's probably fine being in tmp and thrown away later. Probably bit if cleanup we can perform later.

upload_stage_rpms Outdated Show resolved Hide resolved
verify_stage_sigs Outdated Show resolved Hide resolved
verify_stage_sigs Show resolved Hide resolved
@evgeni evgeni merged commit a753ba6 into master Sep 13, 2024
2 of 3 checks passed
@evgeni evgeni deleted the ensure-everything-signed branch September 13, 2024 09:56
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.

3 participants