Skip to content

tests/default-network-behavior-change: Adapt to RHCOS 4.11#1511

Merged
cgwalters merged 1 commit intocoreos:testing-develfrom
cgwalters:initrd-testing2
Feb 10, 2022
Merged

tests/default-network-behavior-change: Adapt to RHCOS 4.11#1511
cgwalters merged 1 commit intocoreos:testing-develfrom
cgwalters:initrd-testing2

Conversation

@cgwalters
Copy link
Copy Markdown
Member

We're going to rebase RHCOS 4.11, which picks up a new NM,
and the new case is actually the same.

BTW, minor rant here: We really need to stop defaulting to writing conditionals
that do if is_fcos() {} else if is_rhcos() {} because about 70%
of the time, this is actually trying to test "RHEL 8" versus "Fedora".

And in this case, what we want to dispatch on is the RHEL8 minor
version. Or even the NetworkManager version.

Anyways for now, because we haven't updated the redhat-release-coreos
package because doing so is painful, dispatch on the OCP version.

@cgwalters
Copy link
Copy Markdown
Member Author

BTW, minor rant here: We really need to stop defaulting to writing conditionals
that do if is_fcos() {} else if is_rhcos() {} because about 70%
of the time, this is actually trying to test "RHEL 8" versus "Fedora".

And to repeat myself again (from e.g. #1405 (comment) ) - we'll soon have "RHCOS9" - and if these conditionals are testing the RHEL version or the sub-component (NM) version, they will Just Work.

miabbott
miabbott previously approved these changes Feb 10, 2022
Copy link
Copy Markdown
Member

@miabbott miabbott left a comment

Choose a reason for hiding this comment

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

Looks sane to me

@cgwalters cgwalters enabled auto-merge (rebase) February 10, 2022 15:34
@travier
Copy link
Copy Markdown
Member

travier commented Feb 10, 2022

LGTM but I think we should discard all the version selection logic now that everybody is using the same config.

@cgwalters
Copy link
Copy Markdown
Member Author

Yeah true, in general since we fork this repo per RHCOS, there's not a lot of value in having the new code support older versions.

@dustymabe
Copy link
Copy Markdown
Member

LGTM but I think we should discard all the version selection logic now that everybody is using the same config.

I'd prefer to leave the logic in place for when things change in the future.

@dustymabe
Copy link
Copy Markdown
Member

We need to update the comments above the definitions of the variables at the top of the script.

@dustymabe
Copy link
Copy Markdown
Member

BTW, minor rant here: We really need to stop defaulting to writing conditionals
that do if is_fcos() {} else if is_rhcos() {} because about 70%
of the time, this is actually trying to test "RHEL 8" versus "Fedora".

Yeah. There is some gray area here. In this case I decided up front that I wanted to know anytime the networking configuration changed in a stream (i.e. 4.9, etc). When the failure occurs we can then decide if that change is OK mid stream or not.

@cgwalters
Copy link
Copy Markdown
Member Author

cgwalters commented Feb 10, 2022

I'd prefer to leave the logic in place for when things change in the future.

OK.

We need to update the comments above the definitions of the variables at the top of the script.

Done


# EXPECTED_INITRD_NETWORK_CFG1
# - used on FCOS 35+ releases
# - used on Fedora 35+ and RHEL 8.5+ releases
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks! update line 45 too?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

OK, though the comments are duplicating the conditionals; I think it's better to have a single source of truth. i.e. I'd lean towards deleting the comments instead. But for now, updated that line too

@dustymabe
Copy link
Copy Markdown
Member

When the failure occurs we can then decide if that change is OK mid stream or not.

IOW - this PR is us deciding that this change is safe for 4.11. If any work needed to be done in other areas we would do it now.

@travier travier disabled auto-merge February 10, 2022 16:53
We're going to rebase RHCOS 4.11, which picks up a new NM,
and the new case is actually the same.

BTW, minor rant here: We really need to stop defaulting to writing conditionals
that do `if is_fcos() {} else if is_rhcos() {}` because about 70%
of the time, this is actually trying to test "RHEL 8" versus "Fedora".

And in this case, what we want to dispatch on is the RHEL8 minor
version.  Or even the NetworkManager version.

Anyways for now, because we haven't updated the `redhat-release-coreos`
package because doing so is painful, dispatch on the OCP version.
Copy link
Copy Markdown
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM

@cgwalters cgwalters enabled auto-merge (rebase) February 10, 2022 17:53
@cgwalters cgwalters disabled auto-merge February 10, 2022 19:11
@cgwalters
Copy link
Copy Markdown
Member Author

Unrelated CI failure, I tested this locally with FCOS and rhcos.

@cgwalters cgwalters merged commit 82f22f9 into coreos:testing-devel Feb 10, 2022
cgwalters added a commit to cgwalters/os that referenced this pull request Feb 10, 2022
Specifically to pull in coreos/fedora-coreos-config#1511

```
Colin Walters (1):
      tests/default-network-behavior-change: Adapt to RHCOS 4.11

CoreOS Bot (1):
      lockfiles: bump to latest

Dusty Mabe (2):
      denylist: snooze ext.config.chrony.dhcp-propagation in rawhide
      denylist: apply snoozes to the branched stream as well

Huijing Hei (1):
      denylist: enable coreos.boot-mirror.luks test
```
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