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

Improve windows setup process #2079

Merged
merged 11 commits into from
Dec 6, 2023
Merged

Conversation

richardebeling
Copy link
Member

@richardebeling richardebeling commented Nov 21, 2023

  • Don't require manual fiddling with autocrlf, instead just enforce it for the repo using a .gitattributes file. This means that users can no longer easily clone to CRLF if they intend to, but I think it's fair for us to do.
  • Enable shallow cloning for the bootstrap submodule (saves ~200MB of network traffic)
  • Add a workaround for ssh not allocating a tty in Git Bash (run ssh with -tt)
  • Don't recommend Git Bash as it behaves weirdly with the vagrant ssh connection with that workaround
    • It doesn't allow auto completion with TAB
    • Pressing Ctrl+C terminates the SSH connection
    • Colors do not work

Open for suggestions regarding the readme content and/or phrasing. You can find the rendered version at https://github.com/richardebeling/EvaP/blob/main/README.md.

TODO:

  • Re-test setup on linux (make sure that nothing was broken)
  • Do we want to replace the clone uri template (currently https) with a ssh template?
    • Https allows quick and easy access to the source code and people can get started with EvaP quickly
    • However, in order to push, I think it makes most sense to just setup an ssh key since GitHub doesn't allow password login anymore
    • Properly setting up an ssh key requires a few considerations that might overwhelm new contributors (passphrase, where to store, ssh-agent), so I think it makes sense to defer that ssh setup to when it is required. People will have to update their remote, but that seems tolerable to me.
    • We could hint at cloning with ssh for users that already have a key set up.

related to #2072

@niklasmohrin
Copy link
Member

Haven't tested, opinions:

  • agree with crlf stuff
  • what does updating bootstrap after we updated upstream look like for everyone else with the shallow clone?
  • if git bash is broken anyways, I think I would even remove the workaround and just discourage using it
  • for cloning, I suppose we could have it in its own step "clone your fork and submodules, for example (https command). Note: to push, you will have to do one more setup step (links to our wiki, which links to GitHub SSH tutorial). The example setup here just gets you started". But its also a bit weird, if we do a "fiddle around with evap" and "contribute" split, we could also postpone the fork step until later. I am not so sure what the best way is, what do you think about the idea in general? I feel that a "git faq" / "how does the git workflow with evap work" wiki page may be nice to have in general

@richardebeling
Copy link
Member Author

  • Bootstrap: I think nothing really changes here. I made a main-updated-boostrap branch to play around with:
    After switching from main to main-with-updated-bootstrap, you get

    $ git status
    [...]
    modified:   evap/static/bootstrap (new commits)
    

    And you can update using

    $ git submodule update
    Submodule path 'evap/static/bootstrap': checked out '344e912d04b5b6a04482113eff20ab416ff01048'
    

    Since we usually update to newer versions of bootstrap, the client will have to fetch new information from the bootstrap repo anyways. "Shallow" only affects (for the average developer useless) information about past versions.

  • Git Bash: I created issues with git-for-windows about broken behavior with vagrant ssh both with their bundled OpenSSH as well as the Windows-provided OpenSSH. I don't think the -tt will realistically cause harm though. You'd have to pipe input through vagrant ssh in order for it to break things. I'd slightly tend towards just leaving it in for now and seeing how the issues play out. Not a strong opinion though.

  • Forking, Pushing, SSH: I like the idea of splitting it up and removing the forking from the initial quick setup. We make people always clone from e-valuation with HTTPS. Further down in the Readme, we can have a new section "Submitting your changes" or something, which would include these steps and explain them:

    • Create the fork through GitHub's UI
    • Set up SSH keys
    • git remote set-url origin git@github.com:<username>/EvaP.git
    • (maybe) git remote add upstream git@github.com:e-valuation/EvaP.git (and possibly update main?)
    • git switch -c <branch_name>
    • git add/commit
    • git push -u origin <branch_name>
    • Go to GitHub, click "Create Pull-Request" button on the huge warning-color banner

    Nice about this:

    1. We have documented the command to create the upstream remote.
    2. We tell people to create a new branch so they don't clobber their main branch.
    3. It moves lots of the Git explanation away from the introduction, so we can skip it there and save time. People are only "bothered" by this once they have successfully improved something. Doing this interactively with first time git users makes sense anyway, as we can spot errors much better.

    I'm not sure how in-depth I want our explanations about the whole fork-pr-workflow and especially the SSH key setup to be. I think I'd rather link to external resources. People will have to figure out their handling of private key passphrases, ssh-agents, integration with password managers, etc. depending on their systems and where they want to be on the security/usability spectrum.

@richardebeling richardebeling force-pushed the main branch 5 times, most recently from 5da3a98 to 634630c Compare November 27, 2023 18:41
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Johannes Wolf <janno42@posteo.de>
Copy link
Collaborator

@Kakadus Kakadus left a comment

Choose a reason for hiding this comment

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

Some parts in the readme are very introductory, but this may be helpful in addition to our presentation for first semesters

Copy link
Member

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

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

Nice

Comment on lines +63 to +64
git remote set-url origin git@github.com:<your-username>/EvaP.git
git remote add upstream git@github.com:e-valuation/EvaP.git
Copy link
Member

Choose a reason for hiding this comment

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

Huh, when did they change that set-url actually sets both fetch and push urls? I thought I remembered that it was odd that you always had to run it once without and once with --push to actually set both urls, but this here works also for me :D

I wonder if we should maybe provide a more idempotent set of commands here:

git remote remove origin
git remote add origin git@github.com:<your-username>/EvaP.git
git remote add upstream git@github.com:e-valuation/EvaP.git

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, when did they change that set-url actually sets both fetch and push urls? I thought I remembered that it was odd that you always had to run it once without and once with --push to actually set both urls, but this here works also for me :D

Fair point. The way I understand the documentation, set-url without any further arguments will simply change the "first" url configured for the remote, and perhaps this is the push and pull url with the trivial setup after cloning? That would mean you'd have to manually set the push url once you specified a separate push url -- that would make sense to me.

I didn't want to mess with the origin branch in any way that could mess up the upstream-branch configuration for main, that's why I wanted to keep origin around but re-point it, but I see the argument about idempotency. How does a branch "remember" its upstream branch, is it just by name, or is it messed up after removing the remote and adding a new remote with the same name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing and re-adding the remote breaks the branch upstream configuration of existing branches. set-url should be as idempotent as remove-add. The main disadvantage of set-url is if multiple URLs are configured, but we don't expect that here (otherwise the user probably knows how remotes work).

Left as set-url for now. We'll see if we run into any trouble with it.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Niklas Mohrin <dev@niklasmohrin.de>
README.md Outdated Show resolved Hide resolved
@richardebeling richardebeling merged commit 6ebfa32 into e-valuation:main Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants