Skip to content

Conversation

hluk
Copy link
Contributor

@hluk hluk commented Oct 8, 2025

Describe your changes

Avoid attaching "-draft" to sidetag names for draft builds.

Relevant Jira

RHELWF-13471

Checklist before requesting a review

  • I have marked as draft or added do not merge label if there's a dependency PR
    • If you want reviews on your draft PR, you can add reviewers or add the release-service-maintainers handle if you are unsure who to tag
  • My commit message includes Signed-off-by: My name <email>
  • I read CONTRIBUTING.MD and commit formatting
  • I have run the README.md generator script in .github/scripts/readme_generator.sh and verified the results using .github/scripts/check_readme.sh

Copy link
Collaborator

@davidmogar davidmogar left a comment

Choose a reason for hiding this comment

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

This LGTM but I'd like to get @mmalina opinion on it.

@hluk hluk force-pushed the fix-importing-sidetags branch 4 times, most recently from 9c4f780 to 9831097 Compare October 10, 2025 12:10
mmalina
mmalina previously approved these changes Oct 10, 2025
@mmalina
Copy link
Contributor

mmalina commented Oct 10, 2025

LGTM, but the tests fail now. Let me know if you need help with that.

@hluk hluk force-pushed the fix-importing-sidetags branch from 9831097 to 6a44268 Compare October 10, 2025 13:00
@elenagerman
Copy link
Contributor

/lgtm, please rebase

davidmogar
davidmogar previously approved these changes Oct 10, 2025
@mmalina
Copy link
Contributor

mmalina commented Oct 10, 2025

@elenagerman FYI, /lgtm doesn't do anything in this repo. You need to use Github UI to approve. Also, there isn't really any need to rebase the PR, unless there are conflicts (which the UI would warn about).

@hluk hluk force-pushed the fix-importing-sidetags branch from 6a44268 to 8f25bbe Compare October 10, 2025 13:55
@hluk
Copy link
Contributor Author

hluk commented Oct 10, 2025

/lgtm, please rebase

Rebased.

@hluk hluk dismissed stale reviews from davidmogar and mmalina via a77ae84 October 10, 2025 14:11
@hluk hluk force-pushed the fix-importing-sidetags branch from 8f25bbe to a77ae84 Compare October 10, 2025 14:11
@hluk
Copy link
Contributor Author

hluk commented Oct 10, 2025

I fixed the failing test: Again problem with bash - we really need to migrate to Python - the bash code is not very maintainable.

Comment on lines 246 to 253
if [[ "$draft" == "true" ]]; then
is_sidetag=$(is_sidetag "$koji_target")
if [[ "$is_sidetag" == "true" ]]; then
koji_tag=${koji_target}
elif [[ "$koji_target" != *-draft ]]; then
koji_tag=${koji_target%-candidate}-draft
fi
else
Copy link
Contributor

Choose a reason for hiding this comment

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

correct me if I'm wrong, but it looks like there's a missing else clause

Suggested change
if [[ "$draft" == "true" ]]; then
is_sidetag=$(is_sidetag "$koji_target")
if [[ "$is_sidetag" == "true" ]]; then
koji_tag=${koji_target}
elif [[ "$koji_target" != *-draft ]]; then
koji_tag=${koji_target%-candidate}-draft
fi
else
if [[ "$draft" == "true" ]]; then
is_sidetag=$(is_sidetag "$koji_target")
if [[ "$is_sidetag" == "true" ]]; then
koji_tag=${koji_target}
elif [[ "$koji_target" != *-draft ]]; then
koji_tag=${koji_target%-candidate}-draft
else
koji_tag=${koji_target}
fi
else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you are right, thanks. I'm thinking about reverting it to the previous version, without the nested ifs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@hluk hluk force-pushed the fix-importing-sidetags branch from a77ae84 to b71a376 Compare October 12, 2025 05:16
Avoid attaching "-draft" to sidetag names for draft builds.

Signed-off-by: Lukas Holecek <[email protected]>
@hluk hluk force-pushed the fix-importing-sidetags branch from b71a376 to 658a5f1 Compare October 12, 2025 05:28
@mmalina
Copy link
Contributor

mmalina commented Oct 13, 2025

I fixed the failing test: Again problem with bash - we really need to migrate to Python - the bash code is not very maintainable.

FWIW, you can create a python script in the utils repo and use it in your task. That way you can have normal python unit tests.

@mmalina
Copy link
Contributor

mmalina commented Oct 13, 2025

/ok-to-test

@elenagerman
Copy link
Contributor

@elenagerman FYI, /lgtm doesn't do anything in this repo. You need to use Github UI to approve. Also, there isn't really any need to rebase the PR, unless there are conflicts (which the UI would warn about).

Thank you @mmalina ! it was just the sign that's all good, and I wait for rebase (+ all tests passe) to approve (if needed) :)

@hluk
Copy link
Contributor Author

hluk commented Oct 14, 2025

Is there any problem with CI again?

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.

4 participants