Skip to content

Refactor RHEL publish #981

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Refactor RHEL publish #981

wants to merge 1 commit into from

Conversation

JackPGreen
Copy link
Collaborator

The publish_rhel script is very long and although split out into functions, I found it difficult to reason exactly what it does.

Refactored by:

  • updated tag_image_push_rhel to call the script directly rather than source-ing them and calling bits of them
  • tidied up some repeated variables at the same time
  • re-ordered function declarations by usage

This PR changes nothing else and doesn't fix anything, it's purely a refactor.

Example execution.

The `publish_rhel` script is very long and although split out into functions, _I found_ it difficult to reason exactly what it _does_.

Refactored by:
- updated `tag_image_push_rhel` to call the script directly rather than `source`-ing them and calling _bits_ of them
- tidied up some repeated variables at the same time

This PR changes _nothing else_ and doesn't fix anything, it's purely a refactor.

Example execution.
@JackPGreen JackPGreen requested review from ldziedziul and nishaatr May 14, 2025 11:21
@JackPGreen JackPGreen requested a review from a team as a code owner May 14, 2025 11:21
Copy link
Contributor

@nishaatr nishaatr left a comment

Choose a reason for hiding this comment

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

LGTM
Suggestion: could move Run preflight scan to the script as well or may be there is a reason why it was left out!

@JackPGreen
Copy link
Collaborator Author

LGTM Suggestion: could move Run preflight scan to the script as well or may be there is a reason why it was left out!

Don't really have a strong opinion either way, but having them as separate steps gives some external visibility on where were at in the job.

@nishaatr
Copy link
Contributor

LGTM Suggestion: could move Run preflight scan to the script as well or may be there is a reason why it was left out!

Don't really have a strong opinion either way, but having them as separate steps gives some external visibility on where were at in the job.

No strong opinion from me also but thought make sense keeping RH interaction/logic nicely contained in one place. Don't believe this will affect visibility.
Additionally, may be we log functions with echo "starting to...". I see the wait functions are missing this perhaps because it does log little later. Another option is to group the logging then it will be more clearer

But not fussed with this!

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