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.

Sign up for GitHub

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

Jump to bottom

postinst:(su) run the Workstation App as mortal user #938

Merged
merged 1 commit into from
Feb 12, 2020
Merged

postinst:(su) run the Workstation App as mortal user #938

merged 1 commit into from
Feb 12, 2020

Conversation

afiune
Copy link

@afiune afiune commented Feb 11, 2020 •
edited
Loading

Description

Update the postinst to run the Workstation App as a mortal user instead of root.

Signed-off-by: Salim Afiune afiune@chef.io

Related Issue

Depends on chef/chef-workstation-app#160

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

  • I have read the CONTRIBUTING document.
  • I have run the pre-merge tests locally and they pass.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commits have been signed-off for the Developer Certificate of Origin.

@afiune afiune requested review from a team as code owners February 11, 2020 19:03
@afiune
Copy link
Author

afiune commented Feb 11, 2020

Launched an ad hoc build

@afiune
Copy link
Author

afiune commented Feb 11, 2020 •
edited
Loading

Uhg, this might be wrong and the right environment variable to use is SUDO_USER

tenor-73325880

@afiune
Copy link
Author

afiune commented Feb 11, 2020

I had a problem running the ad hoc build, the error message is:

Feb 11 13:57:12 Salims-MBP installd[1191]: ./postinstall: /opt/chef-workstation/bin/chef_workstation_app_launcher:set:20: no such option: u

I think I need to fix the chef_workstation_app_launcher script.

@afiune afiune mentioned this pull request Feb 11, 2020
Merged
10 tasks
@afiune
Copy link
Author

afiune commented Feb 11, 2020

New ad hoc build here!

marcparadise
marcparadise reviewed Feb 11, 2020
View reviewed changes
omnibus/package-scripts/chef-workstation/postinst Outdated
@@ -58,7 +58,11 @@ if is_darwin; then

# Restart the app if it was running.
echo "Restarting Chef Workstation App..."
$INSTALLER_DIR/bin/$app_launcher load
echo "USER: $USER"
Copy link
Member

Choose a reason for hiding this comment

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

Could we remove the echos? The output is not actionable or informative to the operator .

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree.

Copy link
Author

@afiune afiune Feb 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

100% agreed!! I need to:

  1. wait for the Chef Workstation App to bump its version
  2. rebase this branch
  3. remove the echos
  4. merge!

This build is just for debugging as the commit message says it. I will remove that commit in a bit. thanks for the prompt review

Copy link
Author

Choose a reason for hiding this comment

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

@jonsmorrow @marcparadise could you review again now? I have removed the echos and rebased this branch! 💯

jonsmorrow
jonsmorrow suggested changes Feb 11, 2020
View reviewed changes
omnibus/package-scripts/chef-workstation/postinst Outdated
@@ -58,7 +58,11 @@ if is_darwin; then

# Restart the app if it was running.
echo "Restarting Chef Workstation App..."
$INSTALLER_DIR/bin/$app_launcher load
echo "USER: $USER"
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree.

omnibus/package-scripts/chef-workstation/postinst Outdated
echo "USER: $USER"
echo "USERNAME: $USERNAME"
echo "SUDO_USER: $SUDO_USER"
echo "HOME: $HOME"
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't appear $HOME is used anywhere?

@afiune
Copy link
Author

afiune commented Feb 11, 2020

#940 Merged the Tray App is now version 0.1.67 with the fix chef/chef-workstation-app#160

@afiune
Copy link
Author

afiune commented Feb 11, 2020

Last ad hoc build! Let us wait for 2 hours for the build and then let us verify the functionality, if everything looks good then l will merge this PR.

https://buildkite.com/chef/chef-chef-workstation-master-omnibus-adhoc/builds/211#e49c6e34-ba27-4cdd-94a7-8ca92ea880a6

marcparadise
marcparadise approved these changes Feb 11, 2020
View reviewed changes
Copy link
Member

@marcparadise marcparadise left a comment

Choose a reason for hiding this comment

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

LGTM, assuming you've verified sudo_user will be set in all normal cases the user has used sudo to install.

@afiune
Copy link
Author

afiune commented Feb 11, 2020

@marcparadise I think that is how omnibus runs the postinst script, isn't it? 🤔

@marcparadise
Copy link
Member

@afiune postinst is run by the platform package manager (rpm, dpkg); omnibus is responsible for putting it in the right place in the package itself.

@afiune
Copy link
Author

afiune commented Feb 11, 2020

Good that I tested!

We need to use USER and not SUDO_USER

Feb 11 16:18:43 Salims-MacBook-Pro installd[1191]: ./postinstall: Restarting Chef Workstation App...
Feb 11 16:18:43 Salims-MacBook-Pro installd[1191]: ./postinstall: USER: afiune
Feb 11 16:18:43 Salims-MacBook-Pro installd[1191]: ./postinstall: USERNAME: 
Feb 11 16:18:43 Salims-MacBook-Pro installd[1191]: ./postinstall: SUDO_USER: 
Feb 11 16:18:43 Salims-MacBook-Pro installd[1191]: ./postinstall: HOME: /Users/afiune
Feb 11 16:18:43 Salims-MacBook-Pro installd[1191]: ./postinstall: su: unknown login:

I will update this PR! 🥇

@afiune afiune force-pushed the afiune/run-workstation-app-as-mortal-user branch 2 times, most recently from 22c4cce to f8804dc Compare February 11, 2020 23:28
postinst:(su) run the Workstation App as mortal user
9e09de0 
Signed-off-by: Salim Afiune <afiune@chef.io>
@afiune afiune force-pushed the afiune/run-workstation-app-as-mortal-user branch from f8804dc to 9e09de0 Compare February 12, 2020 05:21
@afiune
Copy link
Author

afiune commented Feb 12, 2020

This is the good one!! This ad hoc build should work!

@afiune afiune merged commit 20a2a66 into master Feb 12, 2020
@chef-expeditor chef-expeditor bot deleted the afiune/run-workstation-app-as-mortal-user branch February 12, 2020 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonsmorrow jonsmorrow jonsmorrow approved these changes

@marcparadise marcparadise marcparadise approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@afiune @marcparadise @jonsmorrow