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

fix(ir-engine): fixed corrupted projects due git http connection #10145

Merged
merged 2 commits into from May 21, 2024

Conversation

jonathan-casarrubias
Copy link
Collaborator

Summary

When using http connections instead of ssh connections, for some reason I faced several errors while trying to install packages using the manifest approach, figured out that directly cloning these projects within packages/projects/projects fixed all these random errors I kept getting, because of all of that trouble I faced I decided to reverse engineer the manifest approach flow and to fix the issue.

As a context I didn't face these issues while staging locally in macos since it only asked me for username and password/token once, while in wsl ubuntu it kept asking me for username and password/token for each project being installed from these manifests, for some reason the credentials reference were lost denying access to clone each project after the second clone in the iteration, in order to fix the problem I enforced SSH connection instead and now it does work as expected.

The second problem was that because of the first problem, while trying to figure out I installed one of these projects using the --branch="main" flag which caused many other errors while trying to run the project locally, since the main branch is outdated and it's supposed to install from the dev branch, but the manifest script wouldn't allow to fix the problem and instead would skip a directory that is already existing, such problem could have been manually fixed by changing the branch on each of the projects having code from an outdated branch which is unproductive and because of that I also introduced a new feature controlled by a new flag --replace="yes" that would allow to decide if override a current installation.

During the process of solving the root cause and improving the user experience, I found a few improvements opportunities like deprecating found antipatterns and unnecessary operations hurting performance.

This PR fixes the HTTP related problems, introduces a new useful feature and Improves performance.

Subtasks Checklist

[x] Enforce SSH connection without breaking installations from manifests having http connections configured
[x] Introduced optional --replace="yes" flag that allows to reinstall a project from a different branch (Default prop value = "no")
[x] Deprecated antipatterns
[x] Improved performance

Breaking Changes

None

References

closes #insert number here

QA Steps

  • Install projects using manifest script in the exact same way is currently expected (validates no breaking changes are introduced)
  • Re-install projects using new --replace="yes" flag using a different branch than originally installed
  • Execute the command git status and git branch from within an installed project to validate that the code has been pulled from the expected branch, also to confirm that the given branch is the only branch locally.

image

@jonathan-casarrubias jonathan-casarrubias added bug Something isn't working javascript Pull requests that update Javascript code labels May 11, 2024
@jonathan-casarrubias jonathan-casarrubias self-assigned this May 11, 2024
Copy link

cla-bot bot commented May 11, 2024

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Jonathan Casarrubias.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

Copy link
Member

@HexaField HexaField left a comment

Choose a reason for hiding this comment

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

Nice improvements, the only thing that comes to mind is still wanting to run fetch from the remote for each project, since devving will usually require checking out branches. My assumption is the user now manually has to fetch with npm run fetch-projects after installing projects, though I could be wrong about what data exists.

@jonathan-casarrubias
Copy link
Collaborator Author

jonathan-casarrubias commented May 11, 2024

@HexaField thanks for the feedback, talking about the need of fetching again right after installing isn't necessary at all, my implementation wouldn't require any checkout or fetch, just for the sake of clarifying I just made a clean installation and then immediately executed $npm run fetch-projects without doing anything after installing the manifests and this is what a dev will see if tries to fetch.

image

I don't know about the lerna message since its completely unrelated, but s you can see, each project is already checked out in the given branch (I did set --branch="dev" in my manifest installation command), and as you can see all projects already are in dev and up to date, so fetching again and rebasing are unnecessary.

Anyway before we merge this, after I finished this work last night and as I went to sleep I kept coding while sleeping lol and realized that unless I'm missing something, a dev still needs to manually run npm install on each of these projects, for a completely new dev that knows nothing about this project (like me during the few past weeks) I had no idea what the install-manifest was internally doing, I had no idea that I needed to run npm install my self, I had no idea why I was getting all of these random errors, so now after reverse engineering this I'm actually advocated to make new devs lifes easier, soooo.... Why aren't we npm installing each project modules from within the manifest install script? can I introduce it? it could be an optional feature like --install-deps="yes" that would install all dependencies for each project if desired, if not a dev could simply not user the --install-deps flag and the script would behave just like is doing it now... Thoughts?

@HexaField
Copy link
Member

I don't think we should add npm i to the script. Even though it would only be necessary to run once after all projects are installed in the root, all these scripts we want to hide behind the control center GUI eventually.

@jonathan-casarrubias
Copy link
Collaborator Author

@HexaField fair enough, although I'd like to propose to mention it in the instructions because that isn't mentioned and any new dev as it is now has to figure out that npm install is still needed after installing these projects, which is unnecessarily time consuming.

Cheers

Copy link
Member

@HexaField HexaField left a comment

Choose a reason for hiding this comment

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

I just tried using this and ran into this issue. Can you provide instructions on how to properly configure git with this workflow?

josh@josh-laptop:~/Desktop/etherealengine$ npm run install-manifest -- --manifestURL="https://github.com/EtherealEnginePro/project-manifest/blob/main/client-support.manifest.json" --branch="dev"

> etherealengine@0.0.0 install-manifest
> cross-env ts-node --swc scripts/install-manifest.ts --manifestURL=https://github.com/EtherealEnginePro/project-manifest/blob/main/client-support.manifest.json --branch=dev

The authenticity of host 'github.com (20.248.137.48)' can't be established.
ED25519 key fingerprint is --redacted--
This key is not known by any other names
Are you sure you want to continue connecting (yes/no/[fingerprint])? yes
Error: Command failed: git clone git@github.com:EtherealEnginePro/project-manifest /home/josh/Desktop/etherealengine/.temp/project-manifest
Cloning into '/home/josh/Desktop/etherealengine/.temp/project-manifest'...
Warning: Permanently added 'github.com' (ED25519) to the list of known hosts.
git@github.com: Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

    at ChildProcess.exithandler (node:child_process:422:12)
    at ChildProcess.emit (node:events:517:28)
    at ChildProcess.emit (node:domain:489:12)
    at maybeClose (node:internal/child_process:1098:16)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:303:5) {
  code: 128,
  killed: false,
  signal: null,
  cmd: 'git clone git@github.com:EtherealEnginePro/project-manifest /home/josh/Desktop/etherealengine/.temp/project-manifest'
}
Cloning into '/home/josh/Desktop/etherealengine/.temp/project-manifest'...
Warning: Permanently added 'github.com' (ED25519) to the list of known hosts.
git@github.com: Permission denied (publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

manifest installed
Error: ENOENT: no such file or directory, open '/home/josh/Desktop/etherealengine/.temp/project-manifest/client-support.manifest.json'
    at Object.openSync (node:fs:596:3)
    at Object.readFileSync (node:fs:464:35)
    at fetchManifest (/home/josh/Desktop/etherealengine/scripts/install-manifest.ts:78:29)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async installManifest (/home/josh/Desktop/etherealengine/scripts/install-manifest.ts:97:21)
    at async Object.<anonymous> (/home/josh/Desktop/etherealengine/scripts/install-manifest.ts:169:5) {
  errno: -2,
  syscall: 'open',
  code: 'ENOENT',
  path: '/home/josh/Desktop/etherealengine/.temp/project-manifest/client-support.manifest.json'
}
ERROR: Failed to fetch manifest

@jonathan-casarrubias
Copy link
Collaborator Author

jonathan-casarrubias commented May 15, 2024

@HexaField Sure no problem, in the case you haven't ever created SSH Keys for your OS user.

1.- Open a terminal window
2.- Run the command ssh-keygen -t ed25519 -C "your_email@example.com" and press Enter
3.- When you're prompted to "Enter a file in which to save the key", you can press Enter to accept the default file location.
4.- At the prompt, "type a secure passphrase" press Enter

All of that should have created your ssh keys, once you have created SSH Keys, copy the contents from the id_rsa.pub and configure github with these keys.

1.- Open a browser window/tab
2.- Go to github.com
3.- Click on photo of your user located top right of the web page.
4.- Click on Settings
5.- Click on SSH and GPG Keys
6.- Click on "New SSH Key"
7.- Add key title and paste id_rsa.pub contents

By following these steps a dev is never asked for Username and Password ever again and connections will be made more secure through SSH protocol instead of deprecated HTTP connection.

Copy link
Member

@HexaField HexaField left a comment

Choose a reason for hiding this comment

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

Working well!

@HexaField HexaField merged commit 4029046 into dev May 21, 2024
13 checks passed
@HexaField HexaField deleted the jc/improve-manifest-script branch May 21, 2024 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working javascript Pull requests that update Javascript code verified-contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants