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

Support for multiple Prefix values in RPM spec. #698

Closed
wants to merge 2 commits into from
Closed

Support for multiple Prefix values in RPM spec. #698

wants to merge 2 commits into from

Conversation

justinnichols
Copy link

Added support for multiple Prefix values in the RPM spec. This is not backward compatible, the interface has changed. Also fixed it so when the RPM spec is written, "prefix:" is now "Prefix:".

@muuki88
Copy link
Contributor

muuki88 commented Nov 14, 2015

cc @fsat @huntc

@muuki88
Copy link
Contributor

muuki88 commented Dec 3, 2015

I would like to merge this, but I have no experience with rpmPrefix after all. This feature has caused some confusions in the passed, so I would like to be extra carefully here.

@justinnichols also could you add some notes to the docs ( RpmPlugin ) on how to use this

@justinnichols
Copy link
Author

@muuki88 I altered/added documentation to the rpm.rst. I hope it is clear and concise.

@justinnichols
Copy link
Author

I'm not sure what the commit quality needs to be (aside from squashing the commits into one). Please advise if I'm needed to do anything here. Thanks!

@fsat
Copy link
Collaborator

fsat commented Dec 6, 2015

Hi @muuki88 and @justinnichols - apologies for not looking at this earlier.

I've looked the PR, and changing rpmPrefix from Option[String] to Seq[String] is only the first step to enable support for multiple RPM prefix.

I believe there are further changes which are required to make this work. Unfortunately the changes are going to be quite involved due to:

  • Code on master making the assumption that there's only 1 rpmPrefix
  • RPM scriptlets and startup script are bundled during package build time
  • Relocation path(s) is only known during package install time
  • But the RPM scriptlets and startup script need to take into account these relocated paths

These are the changes required.

1. RPM pre-inst scriptlet

https://github.com/justinnichols/sbt-native-packager/blob/c957fdfcf2fc6f05cc2234eef0552f79fa7b3ab7/src/main/resources/com/typesafe/sbt/packager/archetypes/java_server/rpm/preinst-template#L13

Modification required:

  • Store the mapping of the original directory and its relocated path as opposed to storing a single value

Why is this required:

  • The mapping will be used by the start script to cd into the relocated install directory
  • The mapping will also be used by the RPM post content scriptlet to ensure the symlinks within the original paths are relocated correctly

2. Start script

https://github.com/justinnichols/sbt-native-packager/blob/c957fdfcf2fc6f05cc2234eef0552f79fa7b3ab7/src/main/resources/com/typesafe/sbt/packager/archetypes/java_server/systemloader/systemv/start-rpm-template#L40

Modification required:

  • Load the relocated path mapping from Step 1
  • Check if default install path is amongst the relocated paths
  • If the default install path is relocated, use the relocated path
  • Otherwise, use default install path

Why is this required:

  • This will ensure the application is started with current working directory pointing to the correct install directory.

3. Relocate symlink

https://github.com/justinnichols/sbt-native-packager/blob/c957fdfcf2fc6f05cc2234eef0552f79fa7b3ab7/src/main/scala/com/typesafe/sbt/packager/rpm/RpmMetadata.scala#L260
https://github.com/justinnichols/sbt-native-packager/blob/c957fdfcf2fc6f05cc2234eef0552f79fa7b3ab7/src/main/scala/com/typesafe/sbt/packager/rpm/RpmMetadata.scala#L287

Modification required:

  • Load the relocated path mapping from Step 1
  • Ensure each the symlinks within relocated path are updated, (i.e. not pointing to the original default location)

Why this is required:

  • If the symlinks are not moved to the relocated path, it will be broken and application may not function correctly.

4. SystemD service file

45bc15f#diff-4cd37cfbb35d81cf7067b4496c0c6a9cR15

Modification required:

  • Load the relocated path mapping from Step 1
  • Ensure each the path within the service file which are relocated path are updated with the correct location, (i.e. not pointing to the original default location)

@muuki88
Copy link
Contributor

muuki88 commented Dec 6, 2015

@justinnichols Would you be able to make these changes to complete this PR?
Regarding the quality of the code this one looks fine.

My wish would be that we have integration tests (#545) with ansible for testing these stuff. However we don't have any infrastructure for this yet :(
So the only kind of tests you can write are that the scripts are generated properly (see sbt-test folder)

@justinnichols
Copy link
Author

I will do everything I can to help make this more complete. Given the holidays and normal work schedule, it may be some time before I would have something for this. Do you want to leave this PR open until then or would you like to close this and then I can submit a new one once I feel I have completed everything @fsat laid out? Also, thank you, @fsat, for your wonderful descriptions of what would need to be done and the pointers to where things are.

@fsat
Copy link
Collaborator

fsat commented Dec 6, 2015

@justinnichols - you're most welcome :-)

Hi @muuki88 & @justinnichols - I updated the description to add Step 4 which is to include systemd's user service file since #711 has been merged into master.

If you decide to keep this PR going, I'd recommend rebase against master.

@muuki88
Copy link
Contributor

muuki88 commented Dec 8, 2015

@justinnichols I will close this PR for a now and we can link it with your new one.
Thanks from my side as well @fsat :)

@muuki88 muuki88 closed this Dec 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants