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

Validate config map and fix bad Fedora IoT test config #1156

Merged
merged 9 commits into from
Jan 22, 2025

Conversation

achilleas-k
Copy link
Member

  1. Follow-up from Fix user and group creation order in ostree deployments #1154 with the same change but for the edge-ami image type.
  2. Add a new test script that validates the build configurations in the config map¹.
  3. Change the iot-ami image type in the config map to iot-qcow2-image. iot-ami is not a valid image type name.

¹ Note that the validation script does not catch all issues with the config map. For
example, the following configuration is considered valid:

{
  "./configs/some-config.json": {
    "distros": [
      "rhel-10*",
    ],
    "image-types": [
      "ami",
      "invalid-type-name"
    ]
  }
}

because it still produces (at least) one valid configuration:

rhel-10.0, ami, any architecture

This is (somewhat) necessary because we also want the following to be
valid:

{
  "./configs/some-config.json": {
    "distros": [
      "rhel-9*",
      "fedora*"
    ],
    "image-types": [
      "iot-commit",
      "edge-commit"
    ]
  }
}

where iot-commit is only valid for fedora* and edge-commit is only valid
for rhel-9*.

Add a group to the edge-ostree-pull-user config with name osbuild and
gid 1002.  The group is assigned to the user in the same config.  This
covers the scenario that causes the bug that was fixed in the previous
commit.

This config is used to build edge-ami images in CI.
@achilleas-k
Copy link
Member Author

I changed some test names in the github actions which means I had to change the required test list for PRs. We'll need to rebase all other open PRs when this is merged.

thozza
thozza previously approved these changes Jan 21, 2025
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

I've added a few nitpicks and a question, but in general, this looks good to me 👍
Extra points for adding a test for the new script 😅

test/scripts/imgtestlib.py Outdated Show resolved Hide resolved
test/scripts/imgtestlib.py Outdated Show resolved Hide resolved
Reads the config map and verifies that:
1. All listed config files exist, and
2. The combination of distros, arches, and image-types produces at least
   one valid build configuration for each config file.

Note that this does not catch all issues with a config-map.  For
example, the following configuration is considered valid:
```json
{
  "./configs/some-config.json": {
    "distros": [
      "rhel-10*",
    ],
    "image-types": [
      "ami",
      "invalid-type-name"
    ]
  }
}
```

because it still produces (at least) one valid configuration:

    rhel-10, ami, any architecture

This is (somewhat) necessary because we also want the following to be
valid:
```json
{
  "./configs/some-config.json": {
    "distros": [
      "rhel-9*",
      "fedora*"
    ],
    "image-types": [
      "iot-commit",
      "edge-commit"
    ]
  }
}
```

where iot-commit is only valid for fedora* and edge-commit is only valid
for rhel-9*.
Install the go dependencies when running pytest because we now call
cmd/list-images when testing validate-config-map.
iot-ami is not a valid image type name.
Test iot-qcow2-image instead.
@achilleas-k
Copy link
Member Author

achilleas-k commented Jan 21, 2025

Dropped the cmd_root. If we need it in the future, we can re-add it.

thozza
thozza previously approved these changes Jan 21, 2025
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

LGTM

@achilleas-k
Copy link
Member Author

Dismissed again. Had to set the GOPROXY on all the github jobs that need it because of an expired cert at the source of one of the dependencies.

@achilleas-k
Copy link
Member Author

GitLab didn't seem to need it. Do we set it on our gitlab runners by default?

Workaround for expired cert at source of indirect dependency
go.opencensus.io/trace

The Fedora tests are run using a container again instead of relying on
the makefile so we can control the environment.
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

FWIW, LGTM

@supakeen supakeen added this pull request to the merge queue Jan 22, 2025
Merged via the queue into osbuild:main with commit 1e9c3ac Jan 22, 2025
18 checks passed
@achilleas-k achilleas-k deleted the verify-config-map branch January 22, 2025 10:20
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