-
Notifications
You must be signed in to change notification settings - Fork 540
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
Refactor installer / vcs creation & configuration #380
Conversation
@crsmithdev thanks for trying to clean this up. I had a chance to skim it (I'll give it a finer pass later). From this one question came up. There are both Here is seems the installer skips confcopy (if not I might have missed it). That's going to create some issues. Does that make sense? |
Yes...that was a source of some confusion for me as well. Would a workable approach be to replace the installer / vcs |
I really like the config structs. Too much organic growth led to some icky function signatures. This is much cleaner. |
I've updated this w/ more careful treatment of config copies. |
Force: options.Force, | ||
Vendor: options.Vendor, | ||
Config: options.Config, | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice the UpdateTracker
is attached to the Installer
instead of the Vcs
. In later calls it's looking for it as a property on Vcs
. Can you make the tracker available in both places?
@crsmithdev would you like me to take over this PR? |
@sectioneight yes, is there anything I need to do in order to transfer / reassign to you? |
I think you need to add me as a collaborator on https://github.com/crsmithdev/glide so I can push the branch associated with this PR |
@mattfarina added the Tracker onto the VCS as well |
This is not ready for merging yet though, this appears to have broken something:
|
OK, functionality should be restored now, but I'd like an extra set of eyes on this given that I'm unfamiliar with the code and the path that was broken doesn't seem to be covered by existing tests. Where would be the best place to add such tests? |
This is now out of sync and about to be superseded by the gps integration work. Closing. |
As discussed in #372 I am in the process of adding support for URL rewrites. When looking at adding an additional configuration file (
.gliderc
or similar, TBD), I noted that it would be difficult to add in support for this under the current installer / vcs structure -- some vcs methods, for example, already have 9 parameters, and adding onto that is just going to pile up debt.So, in preparation for doing that, I've refactored the installer / vcs code a bit to make creating and configuring them a bit more straightforward, and adjusted the method signatures accordingly. This should make adding a new config file and passing config around much more straightforward.
I think there's certainly additional work to be done (further refactors, more testable abstractions, tests) but this seems like a good stopping point for review.