Only check for the manifest.yaml/image.yaml being used

[mtalexan]

The default manifest.yaml and image.yaml were checked to confirm they exist before it was even known whether they were the right files to use or not. Move the check to occur after we know we're not using a VARIANT that points to different files.

---

This is a minor fix, but it allows custom OS users to conditionally generate manifest{,-$variant}.yaml and/or image{,-$variant}.yaml files. As it is right now, the default manifest.yaml and image.yaml must always exist, even if they're never going to be use by the build (i.e. VARIANT was set with the --variant option).

[openshift-ci]

Hi @mtalexan. Thanks for your PR.

I'm waiting for a coreos member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[dustymabe]

I think the way we've operated so far is that the default will always exist. In a repo with multiple variants those files are just symlinks to whatever current latest development target is.. i.e. take a look at https://github.com/openshift/os.git

[mtalexan]

@dustymabe

I'm pretty sure you're correct that the current builds always have a manifest.yaml and image.yaml present. This bug has been in the codebase for a very long time, and hasn't caused problems with the automated builds at any point.

The --variant CLI option that's been on most of the existing commands for years however, exclusively looks at files named manifest-$VARIANT.yaml and image-$VARIANT.yaml and never looks at manifest.yaml or image.yaml at any point during the build. It's certainly an option to handle variants by replacing the default manifest.yaml and image.yaml with symlinks to different files before running the build instead, but it is an alternative to the existing --variant= option.

I suspect this bug was just hidden because any use cases that might have once included --variant=... were only ever run on a repo that also had the default image.yaml and manifest.yaml files present.

---

I've had a fork of the coreos-assembler for a long while with this fix in. I call it from a homegrown wrapper tool that generates custom manifest YAML files and includes --variant support. My use case prefers the --variant solution rather than the symlink solution (which I did explore). I've just been slow in submitting this fix as a PR until now is all

I understand your hesistant to accept any PRs for things that aren't actually used by osbuild integration, but I figured this would be a trivial enough change it could get accepted anyway.

[mtalexan]

BTW, the change is just a cut-paste of the default file-check logic from occurring immediately before we check if we're using a non-default variant (and therefore completely different files) to being an elif of the non-default variant check.

I've been running with this in my private fork for a couple years without issues.

Pragmatically, either --variant isn't functional despite being present on most of the commands and working in my experience for the last few years, or it is working. If it's not working, this PR has no risk because setting --variant=... doesn't work/isn't supported anyway. If it is working, moving this check will avoid an obviously incorrect unnecessary check only when --variant=... is set and otherwise has no impact.

[mtalexan]

@dustymabe @travier
I think the error message may have been making too many assumptions in the first place. It assumes the only way the manifest.yaml and/or image.yaml could be missing when they should be there is if the caller has a completely custom configuration (configuration != variant).

Probably a better message would be to simply state what requirment wasn't met without any assumptions about how it might have occurred. E.g. "Missing required manifest.yaml and/or image.yaml"

[dustymabe]

"Missing required manifest.yaml and/or image.yaml"

sure - WFM.. though not sure if it's and/or - probably just and ?

[mtalexan]

"Missing required manifest.yaml and/or image.yaml"

sure - WFM.. though not sure if it's and/or - probably just and ?

The check being performed is for whether either file is missing. It technically is "and/or", because either or both files missing will get to the error message. But "or" is probably clear enough.

[dustymabe]

The check being performed is for whether either file is missing. It technically is "and/or", because either or both files missing will get to the error message. But "or" is probably clear enough.

you are correct.

[dustymabe]

/ok-to-test

[openshift-ci]

@mtalexan: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/images	bf65116	link	true	/test images

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.