Skip to content

binary extension migration#4588

Merged
vilmibm merged 1 commit intotrunkfrom
bin-ext-migrate
Nov 17, 2021
Merged

binary extension migration#4588
vilmibm merged 1 commit intotrunkfrom
bin-ext-migrate

Conversation

@vilmibm
Copy link
Contributor

@vilmibm vilmibm commented Oct 21, 2021

Part of #4194

This PR:

  • adds ability to parse ghrepo.Interface and git remotes for a git repo at an arbitrary path (ghrepo.FromPath)
  • Checks all non-binary extensions to see if they have switched to the releases-based precompiled extensions format during upgrade
  • Removes and re-installs any migrated extensions
  • does not worry about the possibility of a git-based extension becoming a binary extension; that is assumed to be a rare event (fixable through a remove/reinstall)

considerations

  • I'm only checking to see if the extension has changed formats when a git upgrade is available; my reasoning is that part of the new format is deleting the gh-* script from the repo's root which would show up as a git upgrade. Is this too hacky? Should we aggressively check for the new binary format even if there is not a git upgrade available?
  • Is the loose error handling sufficient? I didn't want the bin format check to fail an upgrade that would otherwise work but I might be wrong.

@vilmibm vilmibm requested review from mislav and samcoe October 21, 2021 19:31
Copy link
Contributor

@samcoe samcoe left a comment

Choose a reason for hiding this comment

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

I like this direction 👍, left just one comment for discussion.

@samcoe
Copy link
Contributor

samcoe commented Oct 22, 2021

Regarding the open questions:

  1. I think that is a reasonable assumption that there would be git changes when an extension author has migrated to binary extensions.
  2. This also makes sense to me. Although I think the most common scenario would be a network/server issue that would result in both the bin format check and the git pull to both fail.

@vilmibm vilmibm marked this pull request as ready for review October 26, 2021 20:18
@vilmibm vilmibm requested a review from a team as a code owner October 26, 2021 20:18
@vilmibm vilmibm requested a review from samcoe October 26, 2021 20:18
@vilmibm vilmibm changed the title RFC: binary extension migration binary extension migration Oct 26, 2021
Copy link
Contributor

@samcoe samcoe 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 great. Left a couple comments, nothing blocking though 👍

Comment on lines +286 to +315
exts, err := m.list(false)
assert.NoError(t, err)
assert.Equal(t, 1, len(exts))
ext := exts[0]
ext.currentVersion = "old version"
ext.latestVersion = "new version"
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that I introduced this pattern so that I could set currentVersion and latestVersion on an stub extension but after realizing how verbose it is I would definitely like to rethink how we could make it easier to set the versions for a stubbed extension. Not something to address in this PR though, just something to think about.

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

Love the approach. Some notes on the added helpers for git remotes

@vilmibm vilmibm requested a review from mislav October 28, 2021 19:58
Copy link
Contributor

@mislav mislav 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! Sorry for the delay 🙇

return remotes, nil
}

func RemotesForPath(path string) (RemoteSet, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't be addressed in this PR, but across different PRs I'm seeing more and more of the SomethingWithPath functions added to the git package. I wonder whether that should motivate us to restructure our git client as an object that could be initialized with a path or without one? Then, if a path was configured, it would automatically perform all git lookups in the specified directory instead of the current directory. After that, we would not need to add specialized "WithPath" functions.

@vilmibm vilmibm merged commit b5d90e1 into trunk Nov 17, 2021
@vilmibm vilmibm deleted the bin-ext-migrate branch November 17, 2021 19:22
@VictorBatta VictorBatta mentioned this pull request Dec 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants