Opened 4 months ago

Closed 4 months ago

#33589 closed enhancement (fixed)

Gitpod: track remote trac branch

Reported by: gh-tobiasdiez Owned by:
Priority: major Milestone: sage-9.6
Component: build Keywords:
Cc: mkoeppe Merged in:
Authors: Tobias Diez Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: bbe8724 (Commits, GitHub, GitLab) Commit: bbe8724fede61a8e52795e48a9ef2c328f338adc
Dependencies: Stopgaps:

Status badges

Description

Currently, gitpod tracks the branch on github as the remote. Thus, one might accidentally changes to github instead of trac. With this ticket, the remote is changed to trac.

Change History (62)

comment:1 Changed 4 months ago by gh-tobiasdiez

  • Status changed from new to needs_review

comment:2 Changed 4 months ago by mkoeppe

I think git remote set-url --push origin git@trac.sagemath.org:sage.git would also be good to add

comment:3 Changed 4 months ago by gh-tobiasdiez

Not sure if that's really a good idea. The git manual contains

Note that the push URL and the fetch URL, even though they can be set differently, must still refer to the same place. What you pushed to the push URL should be what you would see if you immediately fetched from the fetch URL. If you are trying to fetch from one place (e.g. your upstream) and push to another (e.g. your publishing repository), use two separate remotes.

Your idea was to prevent accidental commits to github, right? Would it be possible to restrict write access for the trac mirror repo to only allow the sync-hook to have write access?

comment:4 Changed 4 months ago by mkoeppe

It's already documented as standard procedure in src/doc/en/developer/manual_git.rst

comment:5 Changed 4 months ago by gh-tobiasdiez

Where exactly? I can only find git remote set-url --push trac git@trac.sagemath.org:sage.git at https://doc.sagemath.org/html/en/developer/manual_git.html

comment:6 Changed 4 months ago by mkoeppe

Yes, that's the one.

comment:7 Changed 4 months ago by gh-tobiasdiez

This is already included in the gitpod config:

      git remote add trac git@trac.sagemath.org:sage.git -t master -t develop -t $(git branch --show-current)
      git remote set-url --push trac git@trac.sagemath.org:sage.git
      git fetch trac
      git branch -u trac/$(git branch --show-current)

comment:8 Changed 4 months ago by mkoeppe

No, the first line is setting the wrong URL for fetching.

comment:9 Changed 4 months ago by gh-tobiasdiez

I took this from the docs: https://doc.sagemath.org/html/en/developer/manual_git.html#the-trac-server

git remote add trac git@trac.sagemath.org:sage.git -t master

What is then the correct url?

comment:10 Changed 4 months ago by mkoeppe

Further up in the same section.

comment:11 Changed 4 months ago by gh-tobiasdiez

git remote add trac https://github.com/sagemath/sagetrac-mirror.git -t master

Does not work well. If you then push something to trac, VS still notifies you that there are changes between your local and remote branch. The reason is that VS invokes git fetch directly after pushing, but then the sync hook has not yet been executed so it still fetches the old branch on github.

comment:12 follow-up: Changed 4 months ago by mkoeppe

OK. Then I'd suggest that we make switching to the ssh remote (git@trac.sagemath.org:sage.git) part of the instructions for adding the SSH key.

comment:13 follow-up: Changed 4 months ago by mkoeppe

We should probably also note this in the documentation (https://doc.sagemath.org/html/en/developer/manual_git.html) that the first configuration is not recommended for users who use VS Code.

comment:14 in reply to: ↑ 12 ; follow-up: Changed 4 months ago by gh-tobiasdiez

Replying to mkoeppe:

OK. Then I'd suggest that we make switching to the ssh remote (git@trac.sagemath.org:sage.git) part of the instructions for adding the SSH key.

The point of this ticket was that this happens automatically for each ticket/gitpod session without manual intervention.

comment:15 in reply to: ↑ 13 ; follow-up: Changed 4 months ago by gh-tobiasdiez

Replying to mkoeppe:

We should probably also note this in the documentation (https://doc.sagemath.org/html/en/developer/manual_git.html) that the first configuration is not recommended for users who use VS Code.

What's the reason for this config in the first place? It is also against the git recommendations that I've cited above.

comment:16 in reply to: ↑ 15 Changed 4 months ago by mkoeppe

Replying to gh-tobiasdiez:

What's the reason for this config in the first place? It is also against the git recommendations that I've cited above.

That's what read-only mirrors are for - they scale better than 1 read/write server.

I don't think that recommendation that you found in the doc is intended to rule out mirrors.

comment:17 in reply to: ↑ 14 Changed 4 months ago by mkoeppe

Replying to gh-tobiasdiez:

Replying to mkoeppe:

OK. Then I'd suggest that we make switching to the ssh remote (git@trac.sagemath.org:sage.git) part of the instructions for adding the SSH key.

The point of this ticket was that this happens automatically for each ticket/gitpod session without manual intervention.

I think we shouldn't force users to do the SSH key thing if all they want to do is fetch from trac.

comment:18 follow-up: Changed 4 months ago by mkoeppe

Maybe it should be made conditional on PRIVATE_SSH_KEY being set already? By the way, the current script in .gitpod.yml creates an empty id_rsa file if PRIVATE_SSH_KEY is not provided; creating that should probably be avoided too

comment:19 Changed 4 months ago by git

  • Commit changed from ac4ec055744192abd4840abe91c520bd13e34ae6 to 77b11ea3ef560b61eaa4ee1031bb0c79a0176ad0

Branch pushed to git repo; I updated commit sha1. New commits:

77b11eaOnly add trac if sshkey is set

comment:20 in reply to: ↑ 18 Changed 4 months ago by gh-tobiasdiez

Replying to mkoeppe:

Maybe it should be made conditional on PRIVATE_SSH_KEY being set already? By the way, the current script in .gitpod.yml creates an empty id_rsa file if PRIVATE_SSH_KEY is not provided; creating that should probably be avoided too

Good suggestion, thanks. I've implemented that now.

comment:21 Changed 4 months ago by mkoeppe

That should be "-n "${PRIVATE_SSH_KEY}"`

comment:22 Changed 4 months ago by gh-tobiasdiez

Yes...

comment:23 follow-up: Changed 4 months ago by mkoeppe

And everyone needs the trac remote. When PRIVATE_SSH_KEY is set, just switch the fetch URL to the good one.

comment:24 Changed 4 months ago by git

  • Commit changed from 77b11ea3ef560b61eaa4ee1031bb0c79a0176ad0 to a5d10a2bef6d2503ed793912217c3ff74a52e84d

Branch pushed to git repo; I updated commit sha1. New commits:

a5d10a2Fix typo

comment:25 in reply to: ↑ 23 Changed 4 months ago by gh-tobiasdiez

Replying to mkoeppe:

And everyone needs the trac remote. When PRIVATE_SSH_KEY is set, just switch the fetch URL to the good one.

But this is origin already, so where is the point of adding trac as the same alias?

comment:26 Changed 4 months ago by mkoeppe

trac is the name used in the developer's guide; and it is also the hardcoded name of the remote that is used in git-trac-command.

comment:27 Changed 4 months ago by git

  • Commit changed from a5d10a2bef6d2503ed793912217c3ff74a52e84d to 4950bcfed0f313db02ca96894a95ccc006df7a15

Branch pushed to git repo; I updated commit sha1. New commits:

4950bcfAdd fallback to github mirror

comment:28 Changed 4 months ago by mkoeppe

+      else
+        # Fallback to sagemath mirror
+        git remote add trac https://github.com/sagemath/sagetrac-mirror.git -t master -t develop
+      fi
+

but only for fetch, not for push!

comment:29 Changed 4 months ago by git

  • Commit changed from 4950bcfed0f313db02ca96894a95ccc006df7a15 to c235eadd675d05d8b8429f72edd76dacdcc5d454

Branch pushed to git repo; I updated commit sha1. New commits:

c235eadDisable pushing to github

comment:30 follow-up: Changed 4 months ago by mkoeppe

This line should probably be run in both cases

+        git remote set-url --push origin pushing-only-via-trac

comment:31 follow-up: Changed 4 months ago by mkoeppe

comment:32 Changed 4 months ago by git

  • Commit changed from c235eadd675d05d8b8429f72edd76dacdcc5d454 to 89591c4a40f185d62846fdc6b1dcd6a7688385dc

Branch pushed to git repo; I updated commit sha1. New commits:

40d1e23Always disable pushing to origin
89591c4Remove wrong trac from prebuild

comment:33 in reply to: ↑ 30 Changed 4 months ago by gh-tobiasdiez

Replying to mkoeppe:

This line should probably be run in both cases

+        git remote set-url --push origin pushing-only-via-trac

Done.

comment:34 in reply to: ↑ 31 Changed 4 months ago by gh-tobiasdiez

Replying to mkoeppe:

Also comment:12, comment:13

I think comment:12 is obsolete now (or I misunderstand it), and comment:13 is not in the scope of this ticket.

comment:35 Changed 4 months ago by mkoeppe

comment:12 - when the user sets up the key the first time (https://620901c077fb7caa9f914f33--sagemath-tobias.netlify.app/developer/workspace.html#section-gitpod), the user needs to change the remote - this needs to be added to the instructions

comment:36 follow-up: Changed 4 months ago by mkoeppe

+        git remote remove trac # might still exists from a previous run/prebuild

does this not give an error if the remote is not there?

comment:37 in reply to: ↑ 36 Changed 4 months ago by gh-tobiasdiez

Replying to mkoeppe:

+        git remote remove trac # might still exists from a previous run/prebuild

does this not give an error if the remote is not there?

Yes, it prints an error (during the prebuild) but the script keeps executing.

comment:38 Changed 4 months ago by mkoeppe

You can silence it by appending 2> /dev/null

comment:39 Changed 4 months ago by git

  • Commit changed from 89591c4a40f185d62846fdc6b1dcd6a7688385dc to e9035ae0330755ccd54bf179a1481fa901b632ac

Branch pushed to git repo; I updated commit sha1. New commits:

3d450f3Update docs
e9035aeSilence semi-expected errors during (pre)build

comment:40 Changed 4 months ago by mkoeppe

I've added comment:13 to #33088

comment:41 Changed 4 months ago by mkoeppe

+ 4. Close this Gitpod workspace.

OK, that's a good solution too

comment:42 follow-up: Changed 4 months ago by mkoeppe

I think all of these steps:

+        git remote remove trac 2> /dev/null # might still exists from a previous run/prebuild

and

+        git remote set-url --push trac git@trac.sagemath.org:sage.git
+        git fetch trac
+        git branch -u trac/$(git branch --show-current)

should be unconditional

comment:43 in reply to: ↑ 42 ; follow-up: Changed 4 months ago by gh-tobiasdiez

Replying to mkoeppe:

I think all of these steps:

+        git remote remove trac 2> /dev/null # might still exists from a previous run/prebuild

During the prebuild there is never a ssh key set, so the trac remote is always either unset or set to the github mirror.

and

+        git remote set-url --push trac git@trac.sagemath.org:sage.git
+        git fetch trac
+        git branch -u trac/$(git branch --show-current)

should be unconditional

Fetching trac doesn't work without ssh keys (at least for me).

comment:44 in reply to: ↑ 43 Changed 4 months ago by mkoeppe

Replying to gh-tobiasdiez:

Fetching trac doesn't work without ssh keys (at least for me).

It doesn't if the remote is ssh, which is why we have changed that.

comment:45 Changed 4 months ago by gh-tobiasdiez

I don't get what you want.

+        git remote set-url --push trac git@trac.sagemath.org:sage.git

is in conflict with

+        git remote set-url --push trac pushing-needs-ssh-key

Since the ssh setup is more involved for gitpod, its better in my opinion to explicitly tell the user that something is off so he might look this up in the docs.

Ps: can we please migrate to github/gitlab; their server are beefy enough to don't need a special mirror setup ;-)

comment:46 Changed 4 months ago by mkoeppe

I mean like this (untested).

diff --git a/.gitpod.yml b/.gitpod.yml
index 7c6800ab8f..8244bafae9 100644
--- a/.gitpod.yml
+++ b/.gitpod.yml
@@ -36,7 +36,9 @@ image:
 tasks:
   - name: Setup
     before: |
+      git remote set-url --push origin pushing-only-via-trac
       # Setup trac as remote
+      git remote remove trac 2> /dev/null # might still exists from a previous run/prebuild
       ## In order to push to trac, generate a new key with `ssh-keygen -f tempkey` and save the private key to gitpod `gp env PRIVATE_SSH_KEY="$(<tempkey)"` (or by following https://www.gitpod.io/docs/environment-variables#using-the-account-settings)
       ## then follow https://doc.sagemath.org/html/en/developer/trac.html#linking-your-public-key-to-your-trac-account to register the public key with trac.
       ## Afterwards, create a new gitpod workspace.
@@ -50,18 +52,15 @@ tasks:
         ssh-keyscan -H trac.sagemath.org >> ~/.ssh/known_hosts
 
         # Setup trac repo
-        git remote remove trac 2> /dev/null # might still exists from a previous run/prebuild
         git remote add trac git@trac.sagemath.org:sage.git -t master -t develop -t $(git branch --show-current)
         git remote set-url --push trac git@trac.sagemath.org:sage.git
-        git fetch trac
-        git branch -u trac/$(git branch --show-current)
-        git remote set-url --push origin pushing-only-via-trac
       else
         # Fallback to sagemath mirror
-        git remote add trac https://github.com/sagemath/sagetrac-mirror.git -t master -t develop
+        git remote add trac https://github.com/sagemath/sagetrac-mirror.git -t master -t develop -t $(git branch --show-current)
         git remote set-url --push trac pushing-needs-ssh-key
-        git remote set-url --push origin pushing-only-via-trac
       fi
+      git fetch trac
+      git branch -u trac/$(git branch --show-current)
       
       ## No need for pyenv
       pyenv shell --unset 2> /dev/null

comment:47 follow-up: Changed 4 months ago by gh-tobiasdiez

That would work, I guess. But I fail to see the point of setting the upstream to "trac" even if you don't have the ssh keys configured. Why change the gitpod default?

I would like to leave the whole trac remote business as minimal as possible.

comment:48 in reply to: ↑ 47 Changed 4 months ago by mkoeppe

Replying to gh-tobiasdiez:

That would work, I guess. But I fail to see the point of setting the upstream to "trac" even if you don't have the ssh keys configured.

Because it means that there are fewer differences between "without ssh key" and "with ssh key", which reduces the complexity in explaining it to developers?

comment:49 Changed 4 months ago by gh-tobiasdiez

If you cannot push anything, then you don't need to worry about trac at all. Everything you need is in "origin". The remove trac is only added because of the limitations of git-trac you mentioned above.

comment:50 Changed 4 months ago by mkoeppe

OK, I think this makes sense

comment:51 follow-up: Changed 4 months ago by mkoeppe

Now if we also want to support a development model in which users use their own GitHub forks as origin, then we should probably do the line git remote set-url --push origin pushing-only-via-trac in the "else" branch only when origin is in the sagemath organization.

comment:52 follow-up: Changed 4 months ago by mkoeppe

And I'd still prefer to move the line

+        git remote remove trac 2> /dev/null # might still exists from a previous run/prebuild

to the very beginning so that it runs in both cases - it's easier to understand that it is correct

comment:53 in reply to: ↑ 51 Changed 4 months ago by gh-tobiasdiez

Replying to mkoeppe:

Now if we also want to support a development model in which users use their own GitHub forks as origin, then we should probably do the line git remote set-url --push origin pushing-only-via-trac in the "else" branch only when origin is in the sagemath organization.

Do you know how/with which credentials the sync hook pushes to the github mirror? Maybe its easy to setup a branch protection rule that would prevent any pushes to the mirror (except for the syncs).

comment:54 Changed 4 months ago by mkoeppe

No idea, sorry

comment:55 Changed 4 months ago by git

  • Commit changed from e9035ae0330755ccd54bf179a1481fa901b632ac to 80e8e2e64d5e9544c61f51b2c527b727b8ae2ee6

Branch pushed to git repo; I updated commit sha1. New commits:

80e8e2eRemove special treatment of origin for pushes

comment:56 Changed 4 months ago by git

  • Commit changed from 80e8e2e64d5e9544c61f51b2c527b727b8ae2ee6 to cc45a3bfaa40905b72527dc1254e1e1e5086e0e4

Branch pushed to git repo; I updated commit sha1. New commits:

0250accTest commit
cc45a3bRevert

comment:57 Changed 4 months ago by gh-tobiasdiez

According to `

` it is sageb0t. I've now added a branch protection rule, see https://groups.google.com/g/sage-devel/c/Z1rx_uP7LtY. Thus the special handling of origin is no longer needed.

Version 0, edited 4 months ago by gh-tobiasdiez (next)

comment:58 in reply to: ↑ 52 Changed 4 months ago by gh-tobiasdiez

Replying to mkoeppe:

And I'd still prefer to move the line

+        git remote remove trac 2> /dev/null # might still exists from a previous run/prebuild

to the very beginning so that it runs in both cases - it's easier to understand that it is correct

I agree.

comment:59 Changed 4 months ago by git

  • Commit changed from cc45a3bfaa40905b72527dc1254e1e1e5086e0e4 to bbe8724fede61a8e52795e48a9ef2c328f338adc

Branch pushed to git repo; I updated commit sha1. New commits:

bbe8724Move reset of trac remote

comment:60 Changed 4 months ago by mkoeppe

  • Reviewers set to Matthias Koeppe
  • Status changed from needs_review to positive_review

comment:61 Changed 4 months ago by gh-tobiasdiez

Thanks for the review and the many helpful suggestions!

comment:62 Changed 4 months ago by vbraun

  • Branch changed from public/build/gitpod_trac_tracking to bbe8724fede61a8e52795e48a9ef2c328f338adc
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.