Pull request in-image (as described in GT book)

[seandenigris]

Inspired by #3819, I tried to follow the instructions in the GT book.

1. The ability to easily recover the repo is a game changer. This was the hardest part and IMO often the barrier to contributions:

GtGitRepositoryRegistryHistory instance repositoryIncludingPackage: 'Lepiter-Extensions'. 
(GtGitRepositoryRegistryHistory instance repositoryNamed: 'lepiter') restore

2. From the issue comments, it seemed that one could use GtGitHubCLITool SSH, which I'd like to do because I am not comfortable with the security implications of storing GH credentials in the image, but when I commented out readTokenFrom: I got an error, and I don't see a way to toggle to SSH.
3. Does aGtGitHubCLITool fork work the same whether a fork is existing or needs to be created? I couldn't test that due to no. 3 above
4. Since I couldn't fork as above, I tried to change the origin in the git tool, but got an opaque error (see image), so I dropped into Morphic to add a remote via Iceberg (note that as a GT missing feature as we consider dropping Morphic)
5. Once I couldn't do the fork in GT, the process devolved and I dropped in the command line to finish. For example, the following failed because it tried to configure the feenk repo, where I don't have rights:

(IceRepository repositoryNamed: 'lepiter')
	createBranch: 'enh_coder-lepiter-doc-view';
	configureUpstreamIfNecessary

Overall, this is an exciting new development and I look forward to being able to fully use it!

[svenvc]

Hi Sean,

First of all, thanks a lot for testing and for the feedback, this is really helpful. Please continue ;-)

Basically, the page How to Glamorous Toolkit through a GitHub pull request needs updating. It was written immediately after the first proof of concept. Since then we got feedback and made improvements, which should hopefully already address all your issues. It should also be possible to drive the whole process mostly through the UI.

GtGitHubCLITool is no longer used and has been replaced by GtGitHubAPITool.

The necessary GitHub personal access token is now accessed through GhTokenHolder. A bit contrary to its name, it is also possible to always read from a file and never actually assign the token to an instance variable in the image. See GhTokenHolder class>>#exampleReadFromFile and the logic in GhTokenHolder class>>#token. So you need to set the path once.

I believe the forking process will handle the case of an already exiting clone as well as a brand new fork that has not yet been checked out. Hopefully it also syncs the fork. This is all in the GitHub API.

So in a brand new ready made GT distribution you have no git repositories. Here you start by browsing the class GtGitRepositoryRegistryHistory, check the Records class view and inspect the repository you need (knowing which repo you need might be a challenge in some situations, hence the package search logic in the original page). Then you can clone or fork with a button.

Now that/those repository/repositories should appear(s) it the GT Git tool. Then you select the underlying inspector tool (click the I on the switcher tool on top) to see the GtGitRepository object. Here you get multiple useful views and actions. The extra actions menu contains both a Fork repository and a Create pull request action. The latter should do all that is needed to complete a PR.

I hope this works for you. Do not hesitate to ask follow up questions.

[svenvc]

https://github.com/feenkcom/gtoolkit/issues/3966

[svenvc]

The documentation has been updated.

[svenvc]

Can we close this ?

[seandenigris]

I’ll try to go through the new instructions today and report back

[seandenigris]

A few questions after going through the new instructions:

1. From @svenvc's comment above, I understood that Libgit-CLI was obsolete, but as of v1.0.1079 there is still a page in the book about it.
2. On the "How to Contribute..." page, the requirements state: "You need… SSH agent setup as well as a personal access token." So am I correct that there is currently no fully in image way to contribute without using an access token? If so, that is not a risk I'm comfortable with, as it seems the unprotected token could still be saved to disk. However, if my fork already exists, it seems GT could switch to that remote via SSH as we can already add remotes in vanilla Iceberg. I think in that case, the whole workflow can be done via SSH in image except maybe the actual PR, which isn't too bad.
3. What is the repository to contribute to the GT Book? While investigating this, I found a few typos, but then realized I couldn't identify the repo via the GtGitRepositoryRegistryHistory instance repositoryIncludingPackage: 'GToolkit-Inspector' package-finding technique described in the above page.

[svenvc]

1. I did not say that, maybe I wasn't clear enough. I said GtGitHubCLITool is no longer used and has been replaced by GtGitHubAPITool. These are tools to work with GitHub's API, we only you them to fork and to do a pull request. Both of these operations you can do manually yourself. It is only for these interactions that the GitHub personal access token is needed.
2. See the last two sentences in 1. On the other hand I am not sure I share your concerns about the security risk of having a token. Of course you are right to be careful. But many tools use such an access token which is limited in scope. Yes it is unprotected. It is often stored in a file, that is one way GhTokenHolder can use it, i.e. without copying it. Do you know a better way ?
3. The repository that contains the GT Book is the main gtoolkit repository.

[seandenigris]

Thanks!

1. So it sounds like the API tool is used by default, but the CLI tool still can be used. I am okay with forking and PR outside the image. The biggest problems have been handled eg creating a branch without wiping changes and restoring the repo
2. Everyone has their own tolerance, but the thing that makes me extra wary with Smalltalk is that running processes can be saved with the image. I don’t have a problem with secrets stored and accessed from files in general
3. Got it thanks

[seandenigris]

Okay, after carefully going through the new instructions, a few points:

Paging over to inspect the Iceberg repository seems convoluted when you have the branch right there in the git tool, however, trying to change the branch via the Magritte form is broken in a few different ways (see video):

1. When the form is not expanded, the branch styling makes it appear editable, when it isn't (not sure if it's clear in the video that I'm trying to edit it to no avail
2. When the form is expanded, typing and accepting a new branch name wipes out the changes in the image - WTH?! Making matters worse, this is not apparent until closing and reopening the repo in the tool. I reported this issue in the quoted part of #3592 and it is still the case. Now that I see that this is not the case when following the instructions to inspect the repository and create the branch from there, I propose that the Magritte form should do whatever the inspector is doing.

For those of us not willing to store a GH token in the image, to change the origin remote to our fork:

• My first instinct was to just change the origin in the git tool. Like the branch field, this field suffers from No. 1 above when the form is not expanded. ~~However, I was pleasantly surprised to see that the former "DNU setOrigin:" error has been fixed and the command line confirms that the origin remote has indeed been changed~~[1]. So it seems the only step not available in the image without a GH token is creating the PR, which is great since that does not require the command line and is fairly straightforward via the web, where we'd often have to go anyway to include photos and videos in the PR description.
• It seems that another method of authenticating gh via token is to pass it directly to gh from a file. This would seemingly avoid ever having it in memory in the image:

  # Authenticate against github.com by reading the token from a file
  $ gh auth login --with-token < mytoken.txt
  

[1]. I could've sworn I had successfully changed a repo's origin via the Git Tool GUI, but now I'm getting the same "DNU setOrigin:". I tried to fix it like this:

GtGitRepository>>#originDescription
	<magritteDescription>
	"..."
				write: [ :aModel :aValue | 
					| remote currentOrigin |
					currentOrigin := aModel repository origin.
					currentOrigin isUndefined
						ifFalse: [ aModel repository removeRemote: currentOrigin ].
					remote := IceGitRemote name: 'origin' url: aValue.
					aModel repository addRemote: remote. "was aModel repository setOriginTo: aValue"
					remote fetch ])

and it almost worked, but I got "IceGitCLIRepository had the subclass responsibility to implement doRemoveRemote:"

https://github.com/user-attachments/assets/c60a75f6-7e3e-4143-a488-3e9ea93dd219

[seandenigris]

@svenvc just making sure you saw my latest comment...

[svenvc]

Regarding branches.

The other branch creation/addition code is in IceRepository>>#gtLocalBranchesFor:

The subtle difference is what should happen if you switch to a (newly created) branch. I agree that uncommitted changes should not be removed. But I understand how things got at this point, i.e. trying to bring the image in sync with what is on disk. I would say that if the reference commit in the image is equal to the commit of the branch, nothing should be (re)loaded, which would be the case for a newly created feature branch.

There is a hierarchy of IceCheckoutStrategy is used here as well.

[svenvc]

Regarding remotes.

I just created a new issue but I am not sure how we should proceed there or how important it is.

I assume that you actually want to swap out the original origin for your fork. So basically you want to edit the origin's url. There is GT specific API for that as well, only used in the MA description (IceLibgitRepository>>#setOriginTo:).

Once you modify the origin you'll want to fetch but like with the branch switching, this might be a delicate operation (unless the commits match).

I am wondering if another, simpler solution could be to just replace the existing repository with a clone of your fork. When the commits match no code should have to be loaded and you would have a working setup straight away.

[seandenigris]

The subtle difference is what should happen if you switch to a (newly created) branch. I agree that uncommitted changes should not be removed. But I understand how things got at this point, i.e. trying to bring the image in sync with what is on disk. I would say that if the reference commit in the image is equal to the commit of the branch, nothing should be (re)loaded, which would be the case for a newly created feature branch.

In Pharo, there is a choice whether to discard changes in the image - after you select "discard image changes" there is another dialog to "checkout" or not, which really means discard image changes, not a git checkout. But there might not be much value added having a two step process because discarding image changes (i.e. reverting code to the commit on disk) would be convenient to have available in the git tool, but should be a separate action because it is needed in other contexts as well. The "holy grail" would be the ability to cherry pick which changes to discard on a per repo basis. Right now, the only levels of granularity available are to manually revert each change or force load each package.

The simplest thing seems like just not altering anything in the image in all cases. Even if the commit on disk doesn't match the reference commit, there may be changes in the image that are still relevant.

RE remotes, it would be nice not to have to drop into the command line for PRs. The stuff you can do in the GH/GL web interfaces isn't too bad but the command line is a draaaag ;-)

[svenvc]

https://github.com/feenkcom/gtoolkit/issues/4039

[JurajKubelka]

@seandenigris , @svenvc : can this issue be closed?

[svenvc]

I think we can close it. Doing a PR now works from inside the image. For specific problems we should create new issues with smaller scopes.