-
-
Notifications
You must be signed in to change notification settings - Fork 148
Implement BASE_URL #1067
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
base: main
Are you sure you want to change the base?
Implement BASE_URL #1067
Conversation
82e8daf to
acc439a
Compare
|
Can you append that to all location where it is needed? we tried that sometime back but it always broke with the subshells. When this is cahnged and working, i will test it furhter. @JamesonRGrieve |
It works for development of new containers as-is (I used it for the ERPNext one) - changing other ones would only be necessary for fork development of core functionality. I'm not super familiar with the core scripts so I'm hesitant to touch them but I can give it a go when I have some more time if you wish. |
|
@MickLesk Do we want the core parts also to be done or work for the moment to allow easy development, where the core (tools.func, core.func ,... ) alsways gets pulled from our github? |
|
Finalize Build.func, i think this week |
|
Thank you for this. I was actually about to open a feature request. In my case, I’m using a fork hosted on my self-hosted Gitea instance (thanks to the community project). Have you tested whether this works with authenticated Gitea instances as well? |
It is not tested and this would need some heavy rework. We need to wait until Mick finishes up with the core changes anyway until we can proceed further with this. |
Happy to help. 😄 |
I mean I've done two new services using it now and it's worked flawlessly so any necessary tweaks should be minor. The quality of life improvement is definitely worth it. |
What you did is easier than what I did. And I even dared to call the script "easily". If you want to check, I think it can be integrated with what you did, since it checks a few other things. https://gist.github.com/AlphaLawless/6d30173b3422d7864aa5e22e633f541c |
The nice thing about the env var method is it's relatively oversight proof - once implemented it's impossible for someone to forget to undo their changes to paths in core files, impossible for a maintainer to miss a change to core files in a code review relating to not reverting paths, etc. Core files never need to be touched to reference forks. |
Scripts wich are clearly AI generated and not further revied by the Author of this PR (in terms of Coding Standards and Script Layout) may be closed without review.
✍️ Description
Implements the calling syntax
export BASE_URL=https://raw.githubusercontent.com/USER/REPO/COMMIT && bash -c "$(curl -fsSL "$BASE_URL"/ct/erpnext.sh)"to allow for deployment of forked scripts without the risk of find-and-replace artifacts leaking. The variable is defaulted to the initial values where I implemented it.Note that I only updated the necessary references for testing development scripts - for full implementation I would recommend similarly updating all references to hard-coded URLs similarly (I noticed they don't all point to the same place, some github, some mirror, etc - intentional?).
I've confirmed this method works while developing the ERPNext container - exact syntax / POSIX compliance / etc at the mercy of maintainers to maintain code standards, will update as needed but this is a HUGE quality of life improvement vs find-and-replace.
✅ Prerequisites (X in brackets)
🛠️ Type of Change (X in brackets)
README,AppName.md,CONTRIBUTING.md, or other docs.🔍 Code & Security Review (X in brackets)
Code_Audit.md&CONTRIBUTING.mdguidelines (updated to match)AppName.sh,AppName-install.sh,AppName.json)