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

Migrate uStreamer directory permissions to the uStreamer Debian package #1467

Closed
jdeanwallace opened this issue Jun 27, 2023 · 14 comments · Fixed by #1546
Closed

Migrate uStreamer directory permissions to the uStreamer Debian package #1467

jdeanwallace opened this issue Jun 27, 2023 · 14 comments · Fixed by #1546
Assignees
Labels

Comments

@jdeanwallace
Copy link
Contributor

jdeanwallace commented Jun 27, 2023

Blocked on https://github.com/tiny-pilot/tinypilot-pro/issues/975

Related #1353

Set the correct permissions of the uStreamer directory (i.e., /opt/ustreamer) using the uStreamer Debian package, instead of using Ansible.

Follow these steps to implement the change and verify that it works as expected:

  1. (For background) Read the first half of the Ansible-to-Debian refactoring doc (until the section "Testing your changes")
  2. In the uStreamer Ansible role, delete the "fix uStreamer folder permissions" Ansible task.
  3. In the uStreamer Debian package, add a step to the postinst script that assigns ownership of the uStreamer directory (i.e., /opt/tinypilot) to the ustreamer user and group (similar to what we do in the TinyPilot Debian package).
  4. Build a new uStreamer Debian package, with your changes, on CircleCI.
  5. In the uStreamer Ansible role, change the ustreamer_debian_package_path to the CircleCI artifact of your uStreamer Debian package with a comment saying it's for temporary testing.
  6. Build a one-off TinyPilot Community install bundle with your changes.
  7. Install the bundle on a bare Raspbian system.
  8. Verify that files in /opt/ustreamer are owned by ustreamer and have group ustreamer.
  9. Create a PR for your uStreamer Debian package changes.
  10. After the PR is reviewed and merged, request that Michael cut a new uStreamer Debian package release.
  11. In the uStreamer Ansible role, update the ustreamer_debian_package_path to be the URL of the newly released uStreamer Debian package that contains your changes.
  12. Create a draft PR from your TinyPilot repo changes.
  13. Add test instructions to your draft PR that allows a teammate to easily test your changes
  14. Submit your PR for review.
  15. After the PR is approved, perform the pre-merge tests on your TinyPilot repo changes.
  16. Merge your PR.
@mtlynch
Copy link
Contributor

mtlynch commented Jul 31, 2023

@db39 - Can you take this on before taking on any new tasks? We've got some Ansible-to-Debian tasks that we need before completing some of the dev work for 2.6.1, so I'd like to prioritize this one, as it will help us test the process of support eng working on this end-to-end.

This is a newly documented process. The dev team has been following kind of ad-hoc processes to do this work for the past year, but we wanted to formalize it more so that the support eng team can participate.

If there are things that are confusing, it's likely a gap in the documentation, so definitely ask about things that don't make sense or places where you get stuck.

@db39
Copy link
Contributor

db39 commented Jul 31, 2023

@mtlynch - Sure!

@db39
Copy link
Contributor

db39 commented Aug 1, 2023

It looks like lintian doesn't like recursive chown calls like the one used in tinypilot.postint. We have an open ticket to avoid setting permissions like this.

The linitian site suggests the following:

There are several ways to mitigate the issue in maintainer scripts:

  • For a static role user, please call chown at build time and not during the installation.
  • If that is too complicated, use runuser(1) in the relevant build parts to create files with correct ownership.
  • Given a static list of files to change, use non-recursive calls for each file. (Please do not generate the list with find.)

What is the best way forward here?

@mtlynch
Copy link
Contributor

mtlynch commented Aug 1, 2023

We can just repeat the same thing we did in the other script and update the bug to say we're doing it in both places.

@db39
Copy link
Contributor

db39 commented Aug 1, 2023

Ok, cool! Is there a way to ignore the warning that breaks the build, or do I have to remove the lintian task?

@mtlynch
Copy link
Contributor

mtlynch commented Aug 1, 2023

We should add a .lintianignore line like this:

tinypilot/.lintianignore

Lines 13 to 15 in 0ef51ca

# Ignore this until we find a better way of setting permissions properly.
# https://github.com/tiny-pilot/tinypilot/issues/1196
recursive-privilege-change

@db39
Copy link
Contributor

db39 commented Aug 2, 2023

It looks like it's working as expected:

pilot@raspberrypi:/opt/ustreamer $ ls -lah
total 48K
drwxr-xr-x 3 ustreamer ustreamer 4.0K Aug  2 12:35 .
drwxr-xr-x 6 root      root      4.0K Aug  2 12:36 ..
drwxr-xr-x 2 ustreamer ustreamer 4.0K Aug  2 12:35 bin
-rw-r--r-- 1 ustreamer ustreamer  35K Aug  1 17:08 LICENSE
lrwxrwxrwx 1 ustreamer ustreamer   13 Aug  1 17:08 ustreamer -> bin/ustreamer
lrwxrwxrwx 1 ustreamer ustreamer   18 Aug  1 17:08 ustreamer-dump -> bin/ustreamer-dump

When I was following the installing a TinyPilot bundle section, I had to follow the manual trigger instructions. I changed the bundle_build_branch to default: << pipeline.git.branch >> - but CircleCI didn't build the bundle for some reason?

I've just requested a review on the uStreamer debian PR. After we merge those changes, I'll continue from step 10.

@jdeanwallace
Copy link
Contributor Author

@db39

I changed the bundle_build_branch to default: << pipeline.git.branch >> - but CircleCI didn't build the bundle for some reason?

Looking at your changes 11af204, it looks like you've changed the base-revision parameter instead of the default parameter.

For example, your change should look something like this:

   bundle_build_branch:
     type: string
     # In order to enable bundle building on a feature branch, you can
     # temporarily change the below default to be: << pipeline.git.branch >>
     # Don’t forget to revert this before merging your branch!
     # For one-off builds, you can also specify this parameter when manually
     # triggering the build pipeline on CircleCI; in this case, the parameter
     # value needs to be the (literal) branch name.
-    default: master
+    default: << pipeline.git.branch >>

With that being said, manually triggering a one-off build is actually easier and slightly prefered, so you're good.

@mtlynch
Copy link
Contributor

mtlynch commented Aug 2, 2023

@db39 - For unexpected CI runs, it's helpful if you can link to the specific run + commit. I think you're talking about this one?

11af204
https://app.circleci.com/pipelines/github/tiny-pilot/tinypilot/3786/workflows/b8933ada-5abe-4a70-ae1e-f68949336fb8

@jdeanwallace - Can you update the instructions? It seems like at the very least default should be backticked so the reader knows we mean that specific YAML key.

@db39
Copy link
Contributor

db39 commented Aug 2, 2023

@mtlynch, @jdeanwallace - Thanks for the info!

I realized my mistake with the base-revision param and fixed it in the following commit: b4cc5e4. It still didn't build a bundle on this workflow.

@jdeanwallace
Copy link
Contributor Author

@db39 - Oh, it look like it did build:

Your change: b4cc5e4
Your workflow with a bundle: https://app.circleci.com/pipelines/github/tiny-pilot/tinypilot/3793/workflows/ae32e331-2005-4701-b5b1-227806788be0

@db39
Copy link
Contributor

db39 commented Aug 2, 2023

@jdeanwallace - yeah that workflow is the one where I used the custom param + manual trigger earlier today.

@jdeanwallace
Copy link
Contributor Author

@db39 - Oh right, I see the time difference now.

db39 added a commit to tiny-pilot/ustreamer-debian that referenced this issue Aug 2, 2023
…ge (#14)

This change sets the correct permissions of the uStreamer directory
(i.e., /opt/ustreamer) using the uStreamer Debian package, replacing the
Ansible task.

Related: tiny-pilot/tinypilot#1467

<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/ustreamer-debian/14"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>
@mtlynch
Copy link
Contributor

mtlynch commented Aug 2, 2023

@db39 - I've cut a ustreamer-debian release for this (step 10 from above):

https://github.com/tiny-pilot/ustreamer-debian/releases/tag/ustreamer_5.38-20230802141939

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 a pull request may close this issue.

3 participants