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

Use upload_stage_rpms to rsync to stagingyum #369

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Nov 1, 2023

This will allow us to align on a single method for the way rsync and upload works and the options used and drive it from theforeman-rel-eng repository for consistency across nightly and releases. See theforeman/theforeman-rel-eng#291 for more details

theforeman.org/pipelines/lib/packaging.groovy Outdated Show resolved Hide resolved
theforeman.org/pipelines/lib/packaging.groovy Outdated Show resolved Hide resolved
@ehelms ehelms marked this pull request as ready for review January 22, 2024 21:22
@ehelms
Copy link
Member Author

ehelms commented Jan 22, 2024

This should be good to go now that the nightly changes were merged to theforeman-rel-eng. I tested this (https://ci.theforeman.org/job/candlepin-nightly-rpm-pipeline/) for Candlepin pipeline. My intent will be to follow this change up with adopting a few other places to use the scripts rather than stand-alone code and centralize the logic into one place.

@ehelms
Copy link
Member Author

ehelms commented Mar 21, 2024

@ekohl could you take another look?

Comment on lines +478 to 480
if (!fileExists('upload_stage_rpms')) {
git url: "https://github.com/theforeman/theforeman-rel-eng", poll: false
}
Copy link
Member

Choose a reason for hiding this comment

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

I know, this is contrary to what Ewoud asked for, but the previous step is always build_stage_repository which comes from the same repo. So I'd argue it's either guaranteed that the repo is already checked out (one could also argue to have a prep step that does only that, for clarity), or whatever is on disk should absolutely not be synced.

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.

It took me a while to wrap my head around this, so I tried to see what I would do when I wrote this. I came up with #437. I think using environment instead of shell exports is a slightly cleaner way.

I was also tempted to write a function that creates the entire Jenkins stage with just 2 parameters.

In the short term I don't mind merging this if we can answer the ssh_key issue I noted inline. I'd also argue that eventually using the secrets mechanism in Jenkins (grep for sshagent) should be a better way because then we don't rely on Puppet providing the SSH key at some known location on some nodes. Not a blocker per se, but what I kept in mind writing the version I did.

}

sh """
export RSYNC_RSH="ssh -i ${ssh_key}"
Copy link
Member

Choose a reason for hiding this comment

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

How is ssh_key defined?

Copy link
Member

Choose a reason for hiding this comment

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

line 476

Copy link
Member

Choose a reason for hiding this comment

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

Bleh, reading diffs is hard.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Blame Richmond.

@ehelms ehelms merged commit acec1c8 into theforeman:master Mar 25, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants