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
Conversation
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Jonathan Casarrubias.
|
There was a problem hiding this 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.
@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 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 |
I don't think we should add |
@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 |
There was a problem hiding this 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
@HexaField Sure no problem, in the case you haven't ever created SSH Keys for your OS user. 1.- Open a terminal window 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 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working well!
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
--replace="yes"
flag using a different branch than originally installedgit status
andgit 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.