-
Notifications
You must be signed in to change notification settings - Fork 2
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
Rework hooks to allow for easier integration with ROCK #5
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. But can you post the set of instructions you used to manually test your changes? 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, 2 questions mainly JFMI.
@@ -0,0 +1,31 @@ | |||
#!/bin/bash | |||
|
|||
set -eux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need trace (x
) by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not required, but it is helpful if there was an error. If the install hook fails, the install fails, otherwise, the output is discarded.
chown -R 584788:root ${SNAP_DATA}/* | ||
chown -R 584788:root ${SNAP_COMMON}/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JFMI only, why we hardcode magic number 584788
instead of snap_daemon
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's for giving flexibility in the rock by not specifying the exact username. This allows us to use the mongo
user in the script by setting the user id manually for both accounts.
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug Fix
What is the current behavior? (You can also link to an open issue here)
The configure hook handled all setup of the snap
What is the new behavior (if this is a feature change)?
Most of the configuration of the snap is done on an install hook and the configuration hook only handles the
snap set
features.Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No.
Other information:
N/A