cmd-buildextend-live: update kargs_json when moving EFI grub.cfg
#3169
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It's more legible that way.
This allows the glob to be relative to the live ISO root. Prep for future patch which relies on this.
Otherwise, code lower down will remove the that `grub.cfg` from the list of kargs files. This in turn will prevent `coreos-installer iso kargs` from affecting kargs on UEFI boots. Fixes 3991e6f ("cmd-buildextend-live: always change dir `EFI/{vendor}/` to correct vendor id").
Rather than silently deleting files that no longer exist, flip this around so that we assert they all exist. This will then ensure that new code moving `grub.cfg` will also have to update `kargs_json`.
|
Let's do coreos/coreos-ci-lib#134 first. |
On FCOS, the EFI directory is already correctly named in the src config repo. So there's no need to do any renames at all. The previous code still worked because `renameat(2)` with the src and dest being the same file is a no-op. This is a bit too implicit though. Let's just add a check to be more clear. This avoids us also unnecessarily iterating over `kargs_json['files']`. While here, add a print statement when this logic kicks in.
|
The reason RHCOS hit this but not FCOS is that in RHCOS we did openshift/os@ Still, I added another commit to make all this more explicit. |
dustymabe
reviewed
Nov 7, 2022
| if len(grubfilepath) != 1: | ||
| raise Exception(f'Found != 1 grub.cfg files: {grubfilepath}') | ||
| srcpath = os.path.dirname(grubfilepath[0]) | ||
| os.renames(srcpath, os.path.join(os.path.dirname(srcpath), vendor_ids[0])) | ||
| if srcpath != f'EFI/{vendor_id}': |
There was a problem hiding this comment.
I feel like this could use a comment.
|
This isn't for this PR (this PR is just fixing an existing problem), but... this whole "rename the directory to a conforming path" feels really weird. It feels like a change that needs to be made somewhere else (C9s?). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Otherwise, code lower down will remove the that
grub.cfgfrom the listof kargs files. This in turn will prevent
coreos-installer iso kargsfrom affecting kargs on UEFI boots.
Fixes 3991e6f ("cmd-buildextend-live: always change dir
EFI/{vendor}/to correct vendor id").