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

Fixes #18539 - Own the public/{assets,webpack} directories #9705

Draft
wants to merge 1 commit into
base: rpm/develop
Choose a base branch
from

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Aug 24, 2023

By owning the directories we can also ensure they are properly cleaned up on upgrade.

This is now a draft to see if this works. If it does, we should also verify if this is a problem with other plugins and apply the same. Probably with macros.

By owning the directories we can also ensure they are properly cleaned
up on upgrade.
@ekohl ekohl force-pushed the rpm/develop-18539-own-all-files branch from f4c5c94 to 020d2c5 Compare August 29, 2024 15:00
@ekohl
Copy link
Member Author

ekohl commented Aug 29, 2024

Confirmed this works:

Before with 3.11 (because katello nightly doesn't work):

# dnf install https://yum.theforeman.org/releases/3.11/el9/x86_64/foreman-release.rpm https://yum.theforeman.org/katello/4.13/katello/el9/x86_64/katello-repos-latest.rpm
# dnf install rubygem-katello
# dnf remove rubygem-katello
# ls /usr/share/gems/gems/katello*
public

After with nightly and the built RPM:

# dnf install https://yum.theforeman.org/katello/nightly/katello/el9/x86_64/katello-repos-latest.rpm https://yum.theforeman.org/releases/nightly/el9/x86_64/foreman-release.rpm
# dnf install https://download.copr.fedorainfracloud.org/results/@theforeman/katello-nightly-staging-scratch-0fbf0e16-ef37-5063-9658-c3bc6540da26/rhel-9-x86_64/07953152-rubygem-katello/rubygem-katello-4.15.0-0.2.pre.master.el9.noarch.rpm
# dnf remove rubygem-katello
# ls /usr/share/gems/gems/katello*
ls: cannot access '/usr/share/gems/gems/katello*': No such file or directory

@ekohl ekohl requested a review from evgeni August 29, 2024 15:21
@ekohl
Copy link
Member Author

ekohl commented Aug 29, 2024

@evgeni before I copy all the plugins, mind taking a look? I don't like that this copies some logic from %foreman_assets_plugin and %foreman_webpack_plugin but other than introducing a new macro I don't know a better way.

@evgeni
Copy link
Member

evgeni commented Aug 30, 2024

How about extending those macros?

diff --git a/packages/foreman/foreman/foreman.spec b/packages/foreman/foreman/foreman.spec
index ffd425623..7cc62953e 100644
--- a/packages/foreman/foreman/foreman.spec
+++ b/packages/foreman/foreman/foreman.spec
@@ -686,10 +686,15 @@ GEMFILE
 %%%{name}_bundlerd_plugin %%{%{name}_bundlerd_dir}/%%{gem_name}.rb
 %%%{name}_pluginconf_dir %{_sysconfdir}/%{name}/plugins
 # Common assets locations
-%%%{name}_assets_plugin %%{gem_instdir}/public/assets/%%{gem_name}
+%%%{name}_assets_plugin \\
+%%dir %%{gem_instdir}/public \\
+%%dir %%{gem_instdir}/public/assets \\
+%%{gem_instdir}/public/assets/%%{gem_name}
 %%%{name}_assets_foreman %%{foreman_dir}/public/assets/%%{gem_name}
 # Common webpack locations
-%%%{name}_webpack_plugin %%{gem_instdir}/public/webpack/%%{gem_name}
+%%%{name}_webpack_plugin \\
+%%dir %%{gem_instdir}/public/webpack \\
+%%{gem_instdir}/public/webpack/%%{gem_name}
 %%%{name}_webpack_foreman %%{foreman_dir}/public/webpack/%%{gem_name}
 # Common apipie locations
 %%%{name}_apipie_cache_plugin %%{gem_instdir}/public/apipie-cache/plugin/%%{gem_name}

@ekohl
Copy link
Member Author

ekohl commented Aug 30, 2024

How about extending those macros?

I also debated introducing a new macro like foreman_plugin_files that takes arguments like foreman_precompile_plugin with which files to include.

Now that I read it, perhaps foreman_precompile_plugin could write out a file that you can pass to %files to include.

@evgeni
Copy link
Member

evgeni commented Aug 30, 2024

It could, but it also sounds more complicated than extending the existing macros?

@ekohl
Copy link
Member Author

ekohl commented Aug 30, 2024

I took a stab at it in #11190 but it really should be easier to build RPMs locally.

@ekohl
Copy link
Member Author

ekohl commented Sep 11, 2024

#11233

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants