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

update commands in agent 'manual install' path #3594

Merged
merged 11 commits into from
Oct 18, 2024
Merged

Conversation

abe-winter
Copy link
Member

@abe-winter abe-winter commented Oct 17, 2024

What changed

  • updates to the 'manual install' path

Why

Ran into trouble when trying it

Manual test

Followed steps in rendered docs on a pi

Questions

  1. Do we actually need this path? Like is the install script not sufficient. (I think the answer is yes, but LMK if not).
  2. Should I nudge people to the 'install viam-agent on live systems' link at the top of the manual install tab? For many users that is actually the better choice

@viambot viambot added the safe to build This pull request is marked safe to build from a trusted zone label Oct 17, 2024
@abe-winter abe-winter marked this pull request as ready for review October 17, 2024 20:50
Copy link
Member

@Otterverse Otterverse left a comment

Choose a reason for hiding this comment

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

Mostly fine, in that it correccts the commands but the one noted issue is that it's not clear this should only be used for offline pre-installs.

Honestly, the manual steps may not even be needed. If the scripted tarball generation isn't enough for a particular client, they can talk to us about why, IMHO. Also, with the unification happening in agent, these instructions will change (and become simpler with only one binary involved) after that releases (monday.)

I'm fine either way though. They've been broken this long and no one else has said anything, so I'm fine with this merging as-is too if you prefer.

@@ -314,15 +314,15 @@ sudo ./preinstall.sh /path/to/rootfs
{{< tablestep >}}
**1. Create and download files**

Run the following commands to create directories for the provisioning binaries, then download the binaries and make them executable:
As root on the target device, run the following commands to create directories for the provisioning binaries, then download the binaries and make them executable:
Copy link
Member

Choose a reason for hiding this comment

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

So the issue is that these instructions are supposed to be when you're NOT on the target device. This is pre-install to a factory image that's not currently running. On a live device, the normal install script should just be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh that makes my experience make more sense. this is the manual version of the preinstall flow

I'm guessing these commands are wrong if you're not on the target device bc of abs paths

Copy link
Collaborator

@npentrel npentrel Oct 17, 2024

Choose a reason for hiding this comment

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

So do any of these changes apply?

Copy link
Member Author

@abe-winter abe-winter Oct 17, 2024

Choose a reason for hiding this comment

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

nope I'm had the wrong idea, changes don't apply

but I think these would still fail in 'editing an sd card' context because of the mix of abs and relative paths

see discussion below about removing the 'manual install' tab entirely -- @npentrel are you okay with that change?

@abe-winter
Copy link
Member Author

abe-winter commented Oct 17, 2024

@Otterverse if your call is to remove this 'manual install' path entirely, and only support the preinstall script, I'm for it. I can make that change in this branch

@Otterverse
Copy link
Member

@Otterverse if your call is to remove this 'manual install' path entirely, and only support the preinstall script, I'm for it. I can make that change in this branch

I think that makes sense. This was orginally deep-dive, last-resort stuff meant for other viam devs trying to understand the provisioning preinstall process. We should instead just focus on improving/fix the script if it ever fails to work for someone (or advanced devs could just read the script to figure out how to do the manual things if they absolutely had to.)

@abe-winter abe-winter marked this pull request as draft October 17, 2024 21:52
@abe-winter abe-winter removed the request for review from npentrel October 17, 2024 21:52
@npentrel
Copy link
Collaborator

That sounds good to me, I'll make the change

@abe-winter
Copy link
Member Author

@npentrel thanks, all yours

@npentrel npentrel force-pushed the agent-manual-install branch from c68aa94 to d6797fd Compare October 18, 2024 16:19
@viambot
Copy link
Member

viambot commented Oct 18, 2024

You can view a rendered version of the docs from this PR at https://docs-test.viam.dev/3594

@npentrel
Copy link
Collaborator

Screenshot 2024-10-18 at 19 03 49

There are some caching issues so providing a screenshot for https://docs-test.viam.dev/3594/how-tos/provision-setup/index.html#install-viam-agent

Does this add better context for the script @abe-winter ?

@npentrel npentrel marked this pull request as ready for review October 18, 2024 17:05
@abe-winter
Copy link
Member Author

yup makes sense now

I think the reason I got it wrong before was that I thought the 'manual install' path was for installing on device not on SD card

@abe-winter
Copy link
Member Author

I can't approve bc github thinks it's my PR, but LGTM

Copy link
Member

@Otterverse Otterverse left a comment

Choose a reason for hiding this comment

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

LGTM

@npentrel
Copy link
Collaborator

Ok - thanks both! merging!

@npentrel npentrel merged commit 2d81f71 into main Oct 18, 2024
10 checks passed
@npentrel npentrel deleted the agent-manual-install branch October 18, 2024 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to build This pull request is marked safe to build from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants