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

put plugin postinstall into a central file, which can be loaded #7226

Merged
merged 2 commits into from Nov 5, 2021

Conversation

evgeni
Copy link
Member

@evgeni evgeni commented Nov 4, 2021

No description provided.

@evgeni
Copy link
Member Author

evgeni commented Nov 4, 2021

this (after 7 years), fixes https://projects.theforeman.org/issues/7988 🎉

this allows plugins to drop most of their postinst scripts
@evgeni evgeni marked this pull request as ready for review November 5, 2021 07:27
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I like the direction. Wouldn't this actually resolve https://projects.theforeman.org/issues/7988?

Comment on lines +4 to +5
PLUGIN_HAS_APIPIE="${3:-true}"
PLUGIN_HAS_WEBPACK="${4:-true}"
Copy link
Member

Choose a reason for hiding this comment

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

Can we be smart? In RPMs we have:

has_assets = spec.files.any? { |f| f.start_with?('app/assets/') }
has_apidoc = spec.files.any? { |f| f =~ %r{\Aapp/controllers/?.*/(api|concerns)/} }
has_cronjob = spec.files.any? { |f| f =~ %r{\Aextra/.*\.cron\z} }
has_webpack = spec.files.any? { |f| f.start_with?('webpack/') }
has_package_json = spec.files.any? { |f| f == 'package.json' }

Should we look for files in $PLUGIN_ROOT and determine the values dynamically if the parameter wasn't passed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we don't need the parameter at all in that case ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

So, something like this:

  # The output sometimes contain additional data like the following line:
  #    Found no changes, using resolution from the lockfile
  # But we really just want the path, so lets grep
  PLUGIN_ROOT="$(${BUNDLE} show ${PLUGIN} | grep '/usr/share/foreman')"
  # Make sure we got a path
  if [ -n "${PLUGIN_ROOT}" ]; then
    if [ -d "${PLUGIN_ROOT}/webpack" ]; then
      mkdir -p $PLUGIN_ROOT/public
      ln -snf /var/lib/foreman/public/webpack/ $PLUGIN_ROOT/public/webpack
    fi
  fi

Copy link
Member Author

Choose a reason for hiding this comment

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

Even tho, looking at the actual plugins, those are all plugins that also have a package.json

Copy link
Member Author

Choose a reason for hiding this comment

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

And calling apipie:cache:index doesn't technically hurt for plugins without API, just takes a bit of time.

Copy link
Member Author

Choose a reason for hiding this comment

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

And because I am already talking to myself: I don't think creating this symlink for plugins that don't have package.json/webpack will actually hurt.

Copy link
Member

Choose a reason for hiding this comment

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

I guess we don't need the parameter at all in that case ;)

I thought it could be useful to override. It's better to have that ability now than to need it at some point and having to go through the dance with updating Foreman and then the plugin.

Even tho, looking at the actual plugins, those are all plugins that also have a package.json

Yes. The only odd one used to be somewhere in Katello where there was a package.json but didn't use webpack. For Debian I think we can ignore that edge case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you absolutely want the "smart" part?

The current layout works and I don't have the time to play around with the smartness for RC1. :(

Copy link
Member

@ekohl ekohl Nov 5, 2021

Choose a reason for hiding this comment

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

I'm OK with postponing smartness. Can we open an issue for that to it doesn't get lost?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added it to https://projects.theforeman.org/issues/7988, as it's still "in progress"

@evgeni
Copy link
Member Author

evgeni commented Nov 5, 2021

To truly solve https://projects.theforeman.org/issues/7988 this needs to be used by all plugins, and then we'd need to drop the duplication in the core packages too ;)

@ehelms
Copy link
Member

ehelms commented Nov 5, 2021

I am good with giving this a go, I think we should do so now or wait till RC1 is out given we are aiming to branch next week Tuesday.

@evgeni
Copy link
Member Author

evgeni commented Nov 5, 2021

I prefer now, as otherwise it means if someone tries a test-upgrade to RC1, they might end up in an error (recoverable, but still).

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I'd also prefer this sooner rather than later. My biggest concern here is that we end up building ruby-foreman-puppet before foreman. Would that still work?

@evgeni
Copy link
Member Author

evgeni commented Nov 5, 2021

@ekohl this is a install-time thing (postinst is just a shell script that runs when dpkg installs the package), so build order doesn't matter.

@evgeni evgeni merged commit 408b48a into theforeman:deb/develop Nov 5, 2021
@evgeni evgeni deleted the drop-plugin-postinst branch November 5, 2021 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants