Feedback items listed during PR1 verification

[vireshnavalli]

1. Personal Access Token : Why is it requiring more than just the repo? : Once this is figured out then we need to update the quick setup guideline to what is needed.

2. I don’t think this part is really needed in the guide (ssh username@WFM-VM-IP 'echo $HOME') because when you run scp it will automatically be in the home directory of the user.

3. It might be good to add an “Exit” choice to all the scripts. The wfm.sh and device-agent.sh script don’t have it but the wfm-cli.sh does.

4. For the wfm-cli.sh it relaunches the script after each options is select so you don’t have to keep launching the wfm.cli.sh We should do the same thing with the wfm.sh and device-agent.sh scripts because it’s helpful.
   5.Use easy CLI – make a header.

5. Instead of requiring commands to call “source <file.env> && sudo –E bash <script.file>” we could include the “source file.env” at the top of each of the three scripts (wfm.sh, wfm-cli.sh and device-agent.sh) to simplify it. If we do this we probably don’t need the “-E” argument either.
   7.All services should have the capability to start on their own after system reboot.

6. Clone the tag while setting up sandbox instead of branch, also we can remove dev_repo_branch references instead use sandbox branch/tag.

7. Change name of the pod/service running on the devices. Within our documentation we have called the service a “Workload Fleet management Client”. Currently when spinning up the service it’s called “Agent”.

8. If a developer spends two days trying to figure out how Symphony is being used, it is time wasted in the sense that Symphony has nothing to do with Margo. So, how do we get developers to focus their attentions on the correct things if they want to learn about Margo. Maybe we could have a table (or something) that maps between the Margo specification items and the code that’s implementing it? I’m not sure what level of granularity, but for example, linking to where the Application Package is being processed, linking to where the desired state API implementation is, linking to the device implementation for reporting desired state and capabilities, etc.

9. The /docs/build.md, /docs/run.md, and /docs/deploy.md do not seem to be as useful since this is all duplicated in the /docs/simplified-setup-guide.md. We should either remove these or put more details in these files that isn’t in the quick start. This document also has a “quickstart” section. We should probably limit the “quickstarts” to just one file: sandbox/pipeline/README.md at main · margo/sandbox. This page really feels like a mostly duplicate of the simplified guide and material and the sandbox/readme.md as well . I think we should consolidate all these documents and rename the “simplified-setup-guide.md” to just “setup-guide.md” because at this point it’s not really a simplification of any existing document. The “/sandbox/docs/simplified-setup-guide.md and "sandbox/readme.md” seems to be the two that make sense. This document also has a “quickstart” section. Maybe we should give it a more specific title since this is for building the Go code, which is different then the simplified setup guide: sandbox/docs/repo-structure.md at main · margo/sandbox

[leste-villecroze-se]

Suggestion: we can create a shallow clone (only fetch 1 commit) then fetch only the relevant branch/tag and check it out.

In the case of checking out the main branch on the margo/sandbox repository, we end up downloading 1.8 MiB instead of 4.3 MiB (which doesn't change much in that case since the repo doesn't have a long history).

Suggested change

	if [[ -n "$GITHUB_USER" && -n "$GITHUB_TOKEN" ]]; then
	git clone "https://${GITHUB_USER}:${GITHUB_TOKEN}@github.com/margo/sandbox.git"
	else
	git clone "https://github.com/margo/sandbox.git"
	fi
	cd "$HOME/sandbox"
	git checkout ${DEV_REPO_BRANCH} || echo 'Branch ${DEV_REPO_BRANCH} not found'
	echo "sandbox repo checkout to branch ${DEV_REPO_BRANCH} done"
	git checkout ${SANDBOX_REPO_BRANCH} || echo 'Branch ${SANDBOX_REPO_BRANCH} not found'
	echo "sandbox repo checkout to branch ${SANDBOX_REPO_BRANCH} done"
	if [[ -n "$GITHUB_USER" && -n "$GITHUB_TOKEN" ]]; then
	git clone --depth 1 "https://${GITHUB_USER}:${GITHUB_TOKEN}@github.com/margo/sandbox.git"
	else
	git clone --depth 1 "https://github.com/margo/sandbox.git"
	fi
	cd "$HOME/sandbox"
	git fetch --depth 1 origin ${SANDBOX_REPO_BRANCH}:${SANDBOX_REPO_BRANCH} || echo 'Unable to fetch ${SANDBOX_REPO_BRANCH}'
	git checkout ${SANDBOX_REPO_BRANCH} || echo 'Branch ${SANDBOX_REPO_BRANCH} not found'
	echo "sandbox repo checkout to branch ${SANDBOX_REPO_BRANCH} done"

[Silvanoc]

I don't fully understand the need to clone the sandbox.

You can get this script without cloning the repo, right. But probably not very common (at least in the beginning).

But if you got it cloning this repo and you want to edit some parts of it, then you need to somehow avoid that an "unmodified" version gets cloned and used.
That is a bad developer experience.

[vireshnavalli]

I don't fully understand the need to clone the sandbox.

You can get this script without cloning the repo, right. But probably not very common (at least in the beginning).

But if you got it cloning this repo and you want to edit some parts of it, then you need to somehow avoid that an "unmodified" version gets cloned and used. That is a bad developer experience.

Hi @Silvanoc you are right but we are using few parts of the sandbox repo to bring up following.
$HOME/sandbox/pipeline/harbor ---> to setup harbor registry and harbor container restart policy.
$HOME/sandbox/poc/tests/artefacts --> to push sample application packages(nextcloud-compose-app, custom-otel-helm-app) to harbor. Here we also build the container image for custom-otel-helm-app and packaging helm chart
$HOME/sandbox/pipeline/observability --> Setting up Prometheus to scrape metrics from multiple devices

cloning is required to get all these files, considering current smaller size of sandbox this shouldn't be an issue but we can try out Sparse Checkout later.

[vireshnavalli]

Suggestion: we can create a shallow clone (only fetch 1 commit) then fetch only the relevant branch/tag and check it out.

In the case of checking out the main branch on the margo/sandbox repository, we end up downloading 1.8 MiB instead of 4.3 MiB (which doesn't change much in that case since the repo doesn't have a long history).

sure @leste-villecroze-se we will check this and update the PR

[leste-villecroze-se]

Thanks @vireshnavalli!

Regarding the comments on whether or not to clone the sandbox, I agree with @Silvanoc: during setup, the user is supposed to have cloned the sandbox already in $HOME/workspace/sandbox or somewhere else. Since this is a prerequisite for running the wfm.sh script, all the files listed are already on disk and we have duplicates:

• The original repo, at $HOME/workspace/sandbox
• The duplicate repo, at $HOME/sandbox

If we want the files to be in the proper location, we can update the setup guide, to ask the user to clone the sandbox in $HOME/sandbox directly, and remove the clone_dev_repo() function entirely. That would make testing local changes in the sandbox easier.

For instance, if I want to change the sample applications locally: in the current setup, I need to uncomment the clone_dev_repo() part in the original repo, then go make my changes in the duplicate repo, then go back to the original repo to run the wfm.sh script (with cloning disabled). This is error-prone! 😱

[vireshnavalli]

@leste-villecroze-se Oh got it, I will change the PR accordingly and update the setup guide.