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

[release-4.6] Fix dns_option typo and add test of container create with DNS option #448

Merged

Conversation

Honny1
Copy link
Member

@Honny1 Honny1 commented Oct 15, 2024

This PR backports fixes a typo in dns_option and adds a DNS option container creation test to version 4.6.

Fixes: https://issues.redhat.com/browse/RHEL-31069

robbmanes and others added 2 commits October 15, 2024 10:06
Signed-off-by: Robb Manes <[email protected]>
(cherry picked from commit 8b2a77d)
Signed-off-by: Jan Rodák <[email protected]>
Signed-off-by: Jan Rodák <[email protected]>
(cherry picked from commit 464dfd8)
Signed-off-by: Jan Rodák <[email protected]>
@inknos
Copy link
Contributor

inknos commented Oct 15, 2024

thanks. pylint is complaining but you could try backporting this commit to make it happy

Two ways of fixing this:
- Ignore R0917 for internal functions such as __int__
- Fix the positional arguments with *

Only exception is for run(image, command=None) for which it is
convenient to run with two positional args.
Example:
    run('fedora:latest', 'ls -la /')

Testing against Pylint stable.
Source of Pylint 3.3.1 docs
https://pylint.readthedocs.io/en/stable/user_guide/messages/refactor/too-many-positional-arguments.html

Signed-off-by: Nicola Sella <[email protected]>
(cherry picked from commit 210c9dc)
Signed-off-by: Jan Rodák <[email protected]>
@Honny1
Copy link
Member Author

Honny1 commented Oct 15, 2024

@inknos Unfortunately this didn't help probably will have to backport the other commits. What commits should I backport next?

@inknos inknos self-requested a review October 15, 2024 13:10
@TomSweeneyRedHat
Copy link
Member

@jwhonce any thoughts on the errors in this PR?

@inknos
Copy link
Contributor

inknos commented Oct 15, 2024

There is a chain of fixes here then here

@Honny1 Honny1 force-pushed the dev/jrodak/dns-options branch 2 times, most recently from f64a8ab to 9f3eee9 Compare October 16, 2024 07:29
inknos and others added 2 commits October 16, 2024 10:15
containers/automation_images#376

Signed-off-by: Nicola Sella <[email protected]>
(cherry picked from commit fd601b2)
Signed-off-by: Jan Rodák <[email protected]>
Signed-off-by: Urvashi Mohnani <[email protected]>
(cherry picked from commit 9b2af6e)
Signed-off-by: Jan Rodák <[email protected]>
@inknos
Copy link
Contributor

inknos commented Oct 16, 2024

since #449 jobs passed I want to give one more run to packit to see what's wrong. Otherwise the PR looks good.

@inknos
Copy link
Contributor

inknos commented Oct 16, 2024

/packit build

@inknos
Copy link
Contributor

inknos commented Oct 16, 2024

the issue looks unrelated to me

/lgtm
/approve

@inknos
Copy link
Contributor

inknos commented Oct 16, 2024

right, I am not approver in this branch. could you take a look? @jwhonce @lsm5

Copy link
Member

@lsm5 lsm5 left a comment

Choose a reason for hiding this comment

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

LGTM. the centos 9 stream aarch64 failure will take a while to go away because of mirror propagation. But x86_64 passed and this is noarch so we're good to go.

/lgtm
/hold

Copy link
Contributor

openshift-ci bot commented Oct 16, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Honny1, inknos, lsm5

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@lsm5
Copy link
Member

lsm5 commented Oct 16, 2024

right, I am not approver in this branch. could you take a look? @jwhonce @lsm5

@inknos Feel free to add yourself. I'll approve and merge.

@inknos
Copy link
Contributor

inknos commented Oct 16, 2024

@lsm5

Just make it go, I'll add myself if needed in the future (I am already approver on main branch) and since Jan did already many rebases (due to pylint failures) I only wish for this PR to be merged 😆

@lsm5
Copy link
Member

lsm5 commented Oct 16, 2024

/hold cancel

@openshift-merge-bot openshift-merge-bot bot merged commit 79d7acd into containers:release-4.6 Oct 16, 2024
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants