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

Effects of sed commands should be moved to patches for easier updating #28

Closed
mkg20001 opened this issue Mar 9, 2024 · 7 comments
Closed

Comments

@mkg20001
Copy link

mkg20001 commented Mar 9, 2024

We've had issues in nixos with iso uploads: NixOS/nixpkgs#294376

The solution was to sed over the files: https://github.com/NixOS/nixpkgs/pull/294497/files

This was because we missed some sed commands.

@stgraber
Copy link
Member

stgraber commented Mar 9, 2024

I'm confused, aren't the files you're sedding the exact same ones that we are?
https://github.com/zabbly/incus/blob/daily/.github/workflows/builds.yml#L330

@cmspam
Copy link

cmspam commented Mar 9, 2024

I am the person who submitted the issue and the pull request to sed over the files.

Just to clarify, the sed commands were taken directly from this repository, as stated in the pull request. To clarify further, the Nix implementation was applying all the patches from zabbly repository, but was not running the sed commands you can see in the builds.yml file linked above by @stgraber which caused broken functionality in the Nix implementation. My fix was to add those sed commands.

Is what @mkg20001 refers to, is that the changes made with the sed commands, should be implemented in patch files instead, or something like that?

@mkg20001
Copy link
Author

mkg20001 commented Mar 9, 2024

The idea was to move the sed commands to the patch files. I didn't know they were from here, but also having them in patch files makes things better for us (package people), since those commands in the github workflow slipped under my radar.

One idea would be to just have a script that generates an additional patch file that's put into git.

@mkg20001
Copy link
Author

mkg20001 commented Mar 9, 2024

Is what @mkg20001 refers to, is that the changes made with the sed commands, should be implemented in patch files instead, or something like that?

Yes

@stgraber
Copy link
Member

stgraber commented Mar 9, 2024

Ah right, so no, we'd rather stick to sed as 99% of the time, the same patterns will work on newer versions of the UI.

Whereas patches basically need manual refreshing every time a new version comes out as simple things like indentation changes will break them.

@mkg20001 mkg20001 changed the title Not all branding replaced / Issues with ISO: Need another run of replacments Effects of sed commands should be moved to patches for easier updating Mar 9, 2024
@mkg20001
Copy link
Author

mkg20001 commented Mar 9, 2024 via email

@stgraber
Copy link
Member

stgraber commented Mar 9, 2024

Yeah, we can move those into a separate sed script.

stgraber added a commit that referenced this issue Mar 26, 2024
Closes #28

Signed-off-by: Stéphane Graber <stgraber@stgraber.org>
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

No branches or pull requests

3 participants