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

Add a gist of the difference between code-server and Coder #4419

Merged
merged 7 commits into from
Nov 9, 2021

Conversation

im-coder-lg
Copy link
Contributor

Fixes #

Here @bpmct I did what I could. Looping in other people who need to review this:

@jsjoeio
@tmikaeld

And all maintainers.

Thanks to @tmikaeld for a small idea of the difference!

@im-coder-lg im-coder-lg requested a review from a team as a code owner October 30, 2021 05:02
@jsjoeio jsjoeio self-assigned this Nov 1, 2021
@jsjoeio jsjoeio added the docs Documentation related label Nov 1, 2021
@jsjoeio jsjoeio added this to the 3.12.2 - improve iPad UX milestone Nov 1, 2021
Copy link
Contributor

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 1, 2021

I'll let @bpmct give the final 👍

@im-coder-lg
Copy link
Contributor Author

Some checks are failing since some things like tokens are failing, what should fix that?

@im-coder-lg
Copy link
Contributor Author

Wait, why's this on the 3.12.2 milestone? This has to be on the 3.12.1 milestone, right? Or, will this be merged after 3.12.1?

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 2, 2021

Some checks are failing since some things like tokens are failing, what should fix that?

The Docs preview is broken for forks unfortunately so you can ignore that (we will fix hopefully soon). The Build/Pre-build shouldn't be failing though 🤔 I just re-ran it

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 2, 2021

This makes no sense 🤔 @code-asher or @TeffenEllis any ideas why the Build is failing here

Traceback (most recent call last):
  File "/opt/hostedtoolcache/node/14.18.1/x64/lib/node_modules/npm/node_modules/node-gyp/gyp/gyp_main.py", line 50, in <module>
    sys.exit(gyp.script_main())
  File "/opt/hostedtoolcache/node/14.18.1/x64/lib/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/__init__.py", line 554, in script_main
    return main(sys.argv[1:])
  File "/opt/hostedtoolcache/node/14.18.1/x64/lib/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/__init__.py", line 547, in main
    return gyp_main(args)
  File "/opt/hostedtoolcache/node/14.18.1/x64/lib/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/__init__.py", line 520, in gyp_main
    [generator, flat_list, targets, data] = Load(
  File "/opt/hostedtoolcache/node/14.18.1/x64/lib/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/__init__.py", line 136, in Load
    result = gyp.input.Load(build_files, default_variables, includes[:],
  File "/opt/hostedtoolcache/node/14.18.1/x64/lib/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/input.py", line 2782, in Load
    LoadTargetBuildFile(build_file, data, aux_data,
  File "/opt/hostedtoolcache/node/14.18.1/x64/lib/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/input.py", line 391, in LoadTargetBuildFile
    build_file_data = LoadOneBuildFile(build_file_path, data, aux_data,
  File "/opt/hostedtoolcache/node/14.18.1/x64/lib/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/input.py", line 263, in LoadOneBuildFile
    LoadBuildFileIncludesIntoDict(build_file_data, build_file_path, data,
  File "/opt/hostedtoolcache/node/14.18.1/x64/lib/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/input.py", line 301, in LoadBuildFileIncludesIntoDict
    LoadOneBuildFile(include, data, aux_data, None, False, check),
  File "/opt/hostedtoolcache/node/14.18.1/x64/lib/node_modules/npm/node_modules/node-gyp/gyp/pylib/gyp/input.py", line 243, in LoadOneBuildFile
    build_file_data = eval(build_file_contents, {'__builtins__': {}},
  File "/home/runner/.cache/node-gyp/14.16.0/include/node/common.gypi", line 43
    # Turn on SipHash for h
                          ^
SyntaxError: unexpected EOF while parsing
gyp ERR! configure error 
gyp ERR! stack Error: `gyp` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onCpExit (/opt/hostedtoolcache/node/14.18.1/x64/lib/node_modules/npm/node_modules/node-gyp/lib/configure.js:351:16)
gyp ERR! stack     at ChildProcess.emit (events.js:400:28)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:282:12)
gyp ERR! System Linux 5.8.0-1042-azure
gyp ERR! command "/opt/hostedtoolcache/node/14.18.1/x64/bin/node" "/opt/hostedtoolcache/node/14.18.1/x64/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd /home/runner/work/code-server/code-server/vendor/modules/code-oss-dev/remote/node_modules/node-pty
gyp ERR! node -v v14.18.1
gyp ERR! node-gyp -v v5.1.0
gyp ERR! not ok

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 2, 2021

Okay this is easy. Can you run yarn fmt, commit changes, rebase and then push up? That should fix this:

image

Copy link
Contributor

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

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

Minor change + yarn fmt

Co-authored-by: Joe Previte <jjprevite@gmail.com>
@im-coder-lg
Copy link
Contributor Author

Will do that soon, need to get my laptop out.

@im-coder-lg im-coder-lg requested a review from jsjoeio November 3, 2021 03:56
@im-coder-lg
Copy link
Contributor Author

Sorry for the delay @jsjoeio it will be here soon, I just had a bunch of problems with Gitpod, the main one being my email, so it took some time to straighten things up, but I did yarn fmt before, and I will need to do it again.

@im-coder-lg
Copy link
Contributor Author

Here this is... Is this correct?

@im-coder-lg
Copy link
Contributor Author

@jsjoeio just curious... The README is in the docs, then how did you loop it into the repo homepage? I mean, that looks impossible, right?

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 3, 2021

just curious... The README is in the docs, then how did you loop it into the repo homepage? I mean, that looks impossible, right?

GitHub let's you put it at the root or in /docs. See here:

If you put your README file in your repository's root, docs, or hidden .github directory, GitHub will recognize and automatically surface your README to repository visitors.

Link

@codecov
Copy link

codecov bot commented Nov 3, 2021

Codecov Report

Merging #4419 (f25768a) into main (94b2774) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4419   +/-   ##
=======================================
  Coverage   66.04%   66.04%           
=======================================
  Files          30       30           
  Lines        1602     1602           
  Branches      315      315           
=======================================
  Hits         1058     1058           
  Misses        466      466           
  Partials       78       78           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94b2774...f25768a. Read the comment docs.

@im-coder-lg
Copy link
Contributor Author

/ping @bpmct requesting review

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 5, 2021

@im-coder-lg sorry, do you mind rebasing one more time? That should fix the e2e check and then I can merge!

@im-coder-lg
Copy link
Contributor Author

Wait, you mean running yarn fmt again?

@im-coder-lg
Copy link
Contributor Author

2021-11-06.09-16-05.mp4

The video up there is the problem I have with yarn fmt. It does work, but then disappears(changes). Do you know what is happening with this?

And yeah, I am using Gitpod because my WIndows PC is slow. It is Windows 7, meaning I use Node 12. So I use Gitpod, and I switched from Node 16 to Node 14 via nvm as shown above. I have the latest yarn version but to no avail. Anybody, any ideas?

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 8, 2021

Wait, you mean running yarn fmt again?

No, rebasing so you have the latest changes from main. This is usually how I do it:

  1. git rebase origin/main
  2. git push --force-with-lease

But since you're on a fork, you'll probably ned to modify to git rebase upstream/main

@im-coder-lg
Copy link
Contributor Author

im-coder-lg commented Nov 8, 2021

I need to fix my brain. I am a Git pro but didn't know the simple thing called rebasing 😂 but thanks for explaining, I will try that now. Should I run yarn fmt again? Maybe I should so doing that now!

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 8, 2021

I need to fix my brain. I am a Git pro but didn't know the simple thing called rebasing 😂 but thanks for explaining, I will try that now.

lol no worries!

Should I run `yarn fly again? Maybe I should so doing that now!

Nope, no need! Rebasing just get you in line with main and the rest of CI should pass (minus Docs preview)

@im-coder-lg
Copy link
Contributor Author

Heads up! Rebase failed. The console logs are here:

asciicast

The asciicast thumbnail shows part of the bug, but check this out:

image
Only an abort fixes that, and there are 5k+ changes so it will be a looong time before I fix it successfully. Any ideas on this?

@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 8, 2021

Looks like @code-asher is here to save the day! He turned on a feature that lets us use GitHub's UI to update your branch. As long as all CI passes, we should be good to merge!

@im-coder-lg
Copy link
Contributor Author

Kinda expected merging through GitHub but thought of using the CLI. I know the GitHub method but didn't understand rebasing fully.

@im-coder-lg
Copy link
Contributor Author

Ah, fixed! Now if the docs issue is fixed(or maybe not), then the PR is ready to be merged!

@im-coder-lg
Copy link
Contributor Author

So, about the docs, what should I do to fix it? I think if you gain access to the fork of mine, you could fix it. For now, I will try studying the Docs CI and find the issue out.

@im-coder-lg
Copy link
Contributor Author

Aha! The first issue here is: https://github.com/cdr/code-server/blob/94b2774f8c9fb120c19232d2739616e447f9b89d/.github/workflows/docs-preview.yaml#L28-L35

The logs in the checks page is:

Run actions/checkout@v2
  with:
    repository: cdr/m
    ref: refs/heads/master
    submodules: true
    fetch-depth: 0
    ssh-strict: true
    persist-credentials: true
    clean: true
    lfs: false
Error: Input required and not supplied: token

The next issue is this: https://github.com/cdr/code-server/blob/94b2774f8c9fb120c19232d2739616e447f9b89d/.github/workflows/docs-preview.yaml#L72-L81

Console logs:

Run ./ci/scripts/github_deployment.sh update
  ./ci/scripts/github_deployment.sh update
  shell: /usr/bin/bash -e {0}
  env:
    GITHUB_DEPLOYMENT: 
    GITHUB_TOKEN: ***
    DEPLOY_STATUS: skipped
    DEPLOY_URL: 
/home/runner/work/_temp/cf73eb14-f128-4001-941c-a1088042e7f8.sh: line 1: ./ci/scripts/github_deployment.sh: No such file or directory
Error: Process completed with exit code 127.

The final one is this:
https://github.com/cdr/code-server/blob/94b2774f8c9fb120c19232d2739616e447f9b89d/.github/workflows/docs-preview.yaml#L83-L94
Console logs:

Run marocchino/sticky-pull-request-comment@v2
  with:
    header: codercom-preview-docs
    message: ✨ Coder.com for PR #4419 deployed! It will be updated on every commit.
  
  * _Host_: /docs/code-server
  * _Last deploy status_: skipped
  * _Commit_: f25768a6c21c0ecf646204d3cdf74711f7182576
  * _Workflow status_: https://github.com/cdr/code-server/actions/runs/1435646749
  
    append: false
    recreate: false
    delete: false
    hide_details: false
    hide: false
    hide_and_recreate: false
    hide_classify: OUTDATED
    GITHUB_TOKEN: ***
Error: Resource not accessible by integration

How can we fix this other than adding the secret tokens(which I don't want to happen since it's a privacy leak of Coder)?

Maybe if this persists, a maintainer only can finish this successfully.

@jsjoeio jsjoeio merged commit 605c3c6 into coder:main Nov 9, 2021
@jsjoeio
Copy link
Contributor

jsjoeio commented Nov 9, 2021

@im-coder-lg nice investigating! Yeah, it's documented here. I need to spend some time soon fixing this - sorry for all your trouble and thanks again for sending this PR ♥️

@jsjoeio jsjoeio modified the milestones: 4.1.0 - improve iPad UX, 4.0.0 Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants