fix: get-app command should be able to clone from any git server #1233

phot0n · 2021-12-08T12:47:31Z

Currently the git remote server is hardcoded to github.com.

Changes:

get-app command now has 2 new options --git-host (for providing from which git host to get the app from - currently only supported for gitlab and github[default]) and --use-ssh - both of these options are to be used with partial url format (previously name tag format) i.e org/repo@branch or org/repo or repo@branch or just repo (the last 2 being for apps under frappe/erpnext org)
support for org/repo format in get-app (rest all partial url formats were already there)
fix for get-app not being able to get app from anywhere but github - if the url provided is valid, we just use it as is for cloning
added a new dependency git-url-parse - which we also use in frappe

closes: #1228

phot0n · 2021-12-08T15:27:26Z

since @Alchez's pr considers the git url format which starts with ssh://, should we add support for that as well?

PS: this format works fine as far as I can test
ssh://git@hostserver/org/repo.git
are there other formats starting with ssh:// for git url?

Alchez · 2021-12-08T15:35:55Z

I would actually suggest using an external dependency (giturlparse) that has already solved the problem of managing git URLs. That handles everything we need, and even spits out URLs for whatever protocol we need.

phot0n · 2021-12-09T14:13:38Z

@Alchez - so had a chat with @ankush , we already use a git parsing library in frappe - https://github.com/coala/git-url-parse - so it'll be better to use the same :)

Alchez · 2021-12-09T14:21:13Z

@phot0n, yeah that works too.

At this point though, I feel like we need to zoom out a bit and look at the design itself. Because we shouldn't even need to extract the host and just simply pass it to the git clone command 😅

The only place I can see where we could need a URL parser is if we wanted to validate if a provided URL is actually a valid git link before processing it. That's what the Frappe app does for the Github connector atleast.

phot0n · 2021-12-09T14:24:39Z

Agreed, had the same thought as to why we're deconstructing and then reconstructing the git url 😅 - though i think it makes sense on shorter slugs where we just pass the repo name and/or branch/tag instead of fully passing the git url

phot0n · 2021-12-09T17:04:02Z

the c872181 will probably fail on python v3.7 test (might fail on all of them 🙈 ) as i've used walrus operator over here (which was added in python v3.8 - will change it)

I've used the git url parsing dependency we use in frappe and did not change the design on how the App Meta class was implemented.
Please do let me know your thoughts @ankush @Alchez :)

Also, please do let me know if any better name pops up for this function :P

bench/utils/__init__.py

bench/app.py

phot0n · 2021-12-10T11:20:50Z

I've updated the pr description and added some screenshots as well :)

TurkerTunali · 2021-12-13T08:26:59Z

IMHO get-app should use exact git clone address. Because get-app is a developer method. Bench should never use healthcare@develop shortcuts. The developer should be freely use his repo and branch like git clone method. It makes harder to get in frappeverse.

phot0n · 2021-12-13T09:52:58Z

@TurkerTunali that option is not new i think as i saw it in previous bench version as well and is only available for first party apps i.e apps present under frappe/erpnext github org(s) and if a git url is passed it will be treated like how git clone treats it (tbh that shortcut is also treated in the same way except we just create the url through code) - as essentially we do use git clone for cloning the repos when we use bench get-app 😅

TurkerTunali · 2021-12-13T13:44:15Z

@phot0n I think we have a litle bit trouble here, but I assume that development is already done. So we may leave it as it is.

phot0n · 2021-12-13T13:53:02Z

@TurkerTunali please do let me know, i don't mind writing it over again :) - this needs to be correct otherwise people won't be able to get their apps 😆

phot0n · 2021-12-14T06:25:29Z

I've added some changes last night, mostly the feature @Alchez talked about here (got a bit carried away 😅 ) - now we can use bench get-app org/repo (and for a specific branch org/repo@branch - which was already there) throughout github by default and also gitlab by providing --git-host gitlab flag. we can also specify if we want to use ssh while cloning the app with this name tag format by providing --use-ssh flag.

Please do let me know your thoughts and also if you want them removed as i guess i went a bit with flow 😅

Alchez

@phot0n, I've suggested some changes (plus documentation changes) that are really nitpicky, but otherwise this is looking good.

As a totally optional side, looking at the code now, I like giturlparse more for us, cause it'll offload the URL validation and generation to the library. But this PR can go as is too.

bench/app.py

gavindsouza · 2021-12-22T05:32:43Z

bench/app.py

+		elif parsed_git_obj and parsed_git_obj.owner:
+			self.tag = self.branch
+			self.org = parsed_git_obj.owner
+			self.repo = parsed_git_obj.name


We could also set self.use_ssh if the passed name was a git SSH url?

it's not necessary i think - it's there to go with the partial url format (when user provides --use-ssh - maybe we can change the naming a bit as it definitely is a bit confusing as to with what it might be used [maybe use_ssh_for_partial_url?])

gavindsouza · 2021-12-22T05:38:19Z

bench/app.py

-			5. healthcare@develop, [email protected]
+			4. frappe/healthcare
+			5. healthcare
+			6. healthcare@develop, [email protected]


I think we can add support for github:frappe/healthcare@version-13 too? SInce we want the gitserver host to be available too 🤔 . We could allow passing TLDs too instead of hardcoding them unconditionally in the get_*_url methods. Eg: git.gav.in:frappe/healthcare@version-13 would also be valid.

tbh i'm not sure ..i was thinking of removing the one i've added as it seemed a bit excessive to me (kinda went with the flow 😅 ) - though if you want it i can try to add that but the host one is kinda solved using --git-host option which is also only for using with partial urls

Passing --git-host is one solution that seems to miss out on the whole idea of why this format was added.

We should get rid of the git_host kwarg altogether. Just fetch this by parsing the url. That's how partial URLs should be handled. Adding a new option doesn't work while integrating this system in the App Dependency System.

hmm, so basically the we want to get the host from the partial format itself rather than providing it through an option - we can do that though i'm not sure how much different they are though cause they basically achieve the same thing 🤔

I think we can add support for github:frappe/healthcare@version-13 too? SInce we want the gitserver host to be available too 🤔 . We could allow passing TLDs too instead of hardcoding them unconditionally in the get_*_url methods. Eg: git.gav.in:frappe/healthcare@version-13 would also be valid.

If we end up supporting hosts in that partial fragment too, I think at that point we might as well expect people to use the full URL anyway, no?

The new get-app is a good option to use when some of the things expected in the URL can be defaulted (host to github, ref to default branch/tag). IMHO, anything beyond that should be received either in the full URL, or as separate options.

+1 for alchez's comment - i think most people don't really care about shortcuts like these - we/they just go to a repo and get the git url and start doing things

gavindsouza · 2021-12-22T05:40:13Z

bench/utils/__init__.py

+		parsed_obj = giturlparse.parse(url)
+	except Exception:
+		# the git url is not parsable
+		return False


Why not let it just raise instead? That's what I'd expect from a util named parse_git_url.

if we look at how it's being used - we'll see that we pass everything coming our way into parse_git_url ..so if the parser raises everytime we'll need to handle that. we can shift the exception handling to setup_details method (?)

The problem I was trying to address here was a naming one. Everything could stay wherever they are if this function is renamed to get_parsed_giturl and returns None instead of False.

ah sorry i misinterpreted 😅 - will do 👍

gavindsouza · 2021-12-27T12:41:37Z

bench/app.py

-			5. healthcare@develop, [email protected]
+			4. frappe/healthcare
+			5. healthcare
+			6. healthcare@develop, [email protected]


Passing --git-host is one solution that seems to miss out on the whole idea of why this format was added.

We should get rid of the git_host kwarg altogether. Just fetch this by parsing the url. That's how partial URLs should be handled. Adding a new option doesn't work while integrating this system in the App Dependency System.

gavindsouza · 2021-12-27T12:48:18Z

bench/utils/__init__.py

+		parsed_obj = giturlparse.parse(url)
+	except Exception:
+		# the git url is not parsable
+		return False


The problem I was trying to address here was a naming one. Everything could stay wherever they are if this function is renamed to get_parsed_giturl and returns None instead of False.

gavindsouza · 2021-12-30T10:25:24Z

I believe the issue this PR was addressing is fixed via #1243. Do we need to salvage anything from here?

phot0n · 2021-12-30T11:22:45Z

@gavindsouza if we have a look at frappe/frappe#15229 - the git url being used there is https://dev.azure.com/user/custom/_git/custom_crm.git which will again fail with the changes in #1243 due to the reason that we're decomposing/recomposing urls (based on the assumption that every url will have the same structure) - it's better to just return/use the url as is if it's a valid git url

(tried on latest develop)

and make get-app work with valid git urls

…url format currently only github and gitlab are supported as git hosts via the git-host option * minor: add logic for picking up branch when using partial url format

* feat(minor): added support for githost in partial url format

gavindsouza · 2022-01-04T11:44:44Z

Seems like giturlparse can't handle azure links either (ref: nephila/giturlparse#6, testing both libraries locally). Raised #1247 to fix the same.

Edit: Updated after realizing Oopsie

Alchez · 2022-01-04T12:13:34Z

Seems like giturlparse can't handle azure links either (ref: nephila/giturlparse#6). Raised #1247 to fix the same.

Just a note cause it seems like package names are to blame here, You've used code from the https://github.com/coala/git-url-parse (v1.2.2) library over at nephila/giturlparse#6 (v0.10.0).

Both of these libraries seem to have their own issues though.

phot0n · 2022-01-05T06:40:45Z

closing this pr in lieu of #1247 - thanks for all the help Alchez, Gavin :)

phot0n marked this pull request as ready for review December 8, 2021 14:32

Alchez suggested changes Dec 10, 2021

View reviewed changes

bench/utils/__init__.py Outdated Show resolved Hide resolved

bench/utils/__init__.py Outdated Show resolved Hide resolved

bench/app.py Outdated Show resolved Hide resolved

phot0n force-pushed the get-host-from-git-url branch from beb6496 to 37a5743 Compare December 10, 2021 10:56

phot0n changed the title ~~fix: get remote server from git url~~ fix: get-app command should be able to clone from any git server Dec 11, 2021

phot0n force-pushed the get-host-from-git-url branch 2 times, most recently from 555b33d to e719c3e Compare December 13, 2021 15:50

Alchez suggested changes Dec 14, 2021

View reviewed changes

bench/app.py Outdated Show resolved Hide resolved

phot0n commented Dec 14, 2021

View reviewed changes

bench/app.py Outdated Show resolved Hide resolved

phot0n force-pushed the get-host-from-git-url branch 3 times, most recently from f07b587 to 5e0492c Compare December 18, 2021 08:39

gavindsouza requested changes Dec 22, 2021

View reviewed changes

gavindsouza requested changes Dec 27, 2021

View reviewed changes

phot0n added 3 commits January 3, 2022 11:37

fix: added git-url-parse lib to parse git url

bb16771

and make get-app work with valid git urls

feat: support for org/repo format when using get-app

fc91ac8

feat: add git-host and use-ssh option to get-app command for partial …

e8f0a52

…url format currently only github and gitlab are supported as git hosts via the git-host option * minor: add logic for picking up branch when using partial url format

chore: remove git-host option from get-app command

65f6055

* feat(minor): added support for githost in partial url format

phot0n force-pushed the get-host-from-git-url branch 2 times, most recently from 38399df to 0f30528 Compare January 3, 2022 09:00

chore: rename parse_git_url to get_parsed_giturl

b9e3e50

phot0n force-pushed the get-host-from-git-url branch from 0f30528 to b9e3e50 Compare January 3, 2022 09:02

phot0n closed this Jan 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: get-app command should be able to clone from any git server #1233

fix: get-app command should be able to clone from any git server #1233

phot0n commented Dec 8, 2021 •

edited

Loading

phot0n commented Dec 8, 2021

Alchez commented Dec 8, 2021

phot0n commented Dec 9, 2021 •

edited

Loading

Alchez commented Dec 9, 2021

phot0n commented Dec 9, 2021 •

edited

Loading

phot0n commented Dec 9, 2021 •

edited

Loading

phot0n commented Dec 10, 2021

TurkerTunali commented Dec 13, 2021

phot0n commented Dec 13, 2021 •

edited

Loading

TurkerTunali commented Dec 13, 2021

phot0n commented Dec 13, 2021

phot0n commented Dec 14, 2021 •

edited

Loading

Alchez left a comment

gavindsouza Dec 22, 2021

phot0n Dec 22, 2021

gavindsouza Dec 22, 2021

phot0n Dec 22, 2021 •

edited

Loading

gavindsouza Dec 27, 2021

phot0n Dec 27, 2021 •

edited

Loading

Alchez Dec 27, 2021

phot0n Dec 27, 2021

gavindsouza Dec 22, 2021

phot0n Dec 22, 2021 •

edited

Loading

gavindsouza Dec 27, 2021

phot0n Dec 27, 2021

gavindsouza Dec 27, 2021

gavindsouza Dec 27, 2021

gavindsouza commented Dec 30, 2021

phot0n commented Dec 30, 2021 •

edited

Loading

gavindsouza commented Jan 4, 2022 •

edited

Loading

Alchez commented Jan 4, 2022 •

edited

Loading

phot0n commented Jan 5, 2022 •

edited

Loading

fix: get-app command should be able to clone from any git server #1233

fix: get-app command should be able to clone from any git server #1233

Conversation

phot0n commented Dec 8, 2021 • edited Loading

phot0n commented Dec 8, 2021

Alchez commented Dec 8, 2021

phot0n commented Dec 9, 2021 • edited Loading

Alchez commented Dec 9, 2021

phot0n commented Dec 9, 2021 • edited Loading

phot0n commented Dec 9, 2021 • edited Loading

phot0n commented Dec 10, 2021

TurkerTunali commented Dec 13, 2021

phot0n commented Dec 13, 2021 • edited Loading

TurkerTunali commented Dec 13, 2021

phot0n commented Dec 13, 2021

phot0n commented Dec 14, 2021 • edited Loading

Alchez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phot0n Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phot0n Dec 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phot0n Dec 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavindsouza commented Dec 30, 2021

phot0n commented Dec 30, 2021 • edited Loading

gavindsouza commented Jan 4, 2022 • edited Loading

Alchez commented Jan 4, 2022 • edited Loading

phot0n commented Jan 5, 2022 • edited Loading

phot0n commented Dec 8, 2021 •

edited

Loading

phot0n commented Dec 9, 2021 •

edited

Loading

phot0n commented Dec 9, 2021 •

edited

Loading

phot0n commented Dec 9, 2021 •

edited

Loading

phot0n commented Dec 13, 2021 •

edited

Loading

phot0n commented Dec 14, 2021 •

edited

Loading

phot0n Dec 22, 2021 •

edited

Loading

phot0n Dec 27, 2021 •

edited

Loading

phot0n Dec 22, 2021 •

edited

Loading

phot0n commented Dec 30, 2021 •

edited

Loading

gavindsouza commented Jan 4, 2022 •

edited

Loading

Alchez commented Jan 4, 2022 •

edited

Loading

phot0n commented Jan 5, 2022 •

edited

Loading