-
Notifications
You must be signed in to change notification settings - Fork 2
Update template-only-bin scripts #29
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
Conversation
This reverts commit ad0263f.
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.
@rocketnova - ran through all the testing instructions and everything worked as expected!
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.
looks great, have some questions about the rename logic and some of the workflows that i couldn't seem to find in this PR
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.
this parameters of this script makes it seem like it supports renaming from any current name to any new name, but i imagine the find/replace logic won't work for some edge cases, like if the current name is something too common.
i know we have a goal of switching to copier soon, where effectively "current name" would become something like {{app_name}}
. have we considered the following options:
- option a: going ahead and naming the current name
{{app_name}}
, and in all template-only github actions workflows, we first run the rename script to an actual app name (e.g.app
) so that CI can run properly (i haven't thought through all of the details to see if this is feasible, but just floating the high level idea first) - option b: reducing the amount of flexibility (and therefore complexity around edge cases) in this script by sticking with an app name like
template-only-app
rather than an app name that differs per application template? then current_name can be hardcoded totemplate-only-app
, and we have more consistency across our application templates (and can further simplify some of the scripts here).
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.
this parameters of this script makes it seem like it supports renaming from any current name to any new name, but i imagine the find/replace logic won't work for some edge cases, like if the current name is something too common.
It currently supports renaming from anything to anything. The regex is restricted to word boundaries, so if you try to rename app
to my-app
, it won't replace happen
with hmy-appen
. But yes, if the user were to name it from app-rails
to class
or else
... they would have some bad times when trying to rename it back to something less common.
i know we have a goal of switching to copier soon,
🤩
option a: going ahead and naming the current name {{app_name}}, and in all template-only github actions workflows, we first run the rename script to an actual app name (e.g. app) so that CI can run properly (i haven't thought through all of the details to see if this is feasible, but just floating the high level idea first)
I have a separate script I'm finishing up that effectively "installs an app" for the infra template, allowing the user to provide the app name as an argument. That's in a forthcoming PR.
option b: reducing the amount of flexibility (and therefore complexity around edge cases) in this script by sticking with an app name like template-only-app rather than an app name that differs per application template? then current_name can be hardcoded to template-only-app, and we have more consistency across our application templates (and can further simplify some of the scripts here).
Per the interest and arguments in navapbc/platform#21, I was sticking with app-*
.
Is the idea behind option b that all the application templates will use template-only-app
and in the installation instructions, the user specifies the name they want to use?
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.
Yeah, the idea behind option b is that all application templates will use template-only-app and force the project team to decide on an app name (which if they really don't want to think about it they can rename it back to something generic like app
). The benefit is to simplify the code path in the install/update paths to limit the amount of dynamic code (the logic for generating template_short_name for example and guaranteeing the "current_name" is the same string in the rename script). I admit it's not a huge simplification.
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.
I'm going to postpone digging into this question until after I've had a chance to make a proof-of-concept of how things might work with copier.
|
||
echo "Storing template version in a file..." | ||
cd "${template_name}" | ||
git rev-parse HEAD >../".${template_name}-version" |
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.
clarification question: is the template version file being stored in the application folder? or the project repo root?
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.
Currently, in the project root dir. Not the application dir. Same as template-application-nextjs
and template-application-flask
.
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.
Thanks for clarifying. I think that poses a problem if a repo has two application using the same application template. Should we consider moving the template file into the app folder?
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.
That's a good point. I agree that we should move the .template-*-version
files to within the app folders. I'll start a 🔒 slack discussion thread about this.
Ticket
Changes
install-template
scriptrename-template-app
scriptupdate-template
script.sh
extensionContext for reviewers
The
install-template
script now:The
rename-template-app
script now:app
no longer will replace the substring inhappens
)This PR adds the
update-template
script:update-template
script diffs between the installed version of the application template (stored in.template-application-rails-version
) and the target version. This is the pattern established on the other Platform templates.Shell script notes
Note that
sed
does not behave consistently cross-platform (i.e. GNU versions installed by default on linux and BSD-ish versions installed by default on macOS). Checking the result of runningsed --version
will reliably distinguish between the two versions. The two main ways this impacts this PR are:sed -i
on linux andsed -i ""
on macOS.\<
and\>
on linux and[[:<:]]
and[[:>:]]
on macOS.Security notes
In general, running
curl <something> | bash -s
is not good security practice. Users should always read the contents of scripts that are piped into bash. However, most users do not. We should switch away from this practice as soon as it is feasible. It opens our repos to becoming a vector for running or installing bad scripts.This PR temporarily hardcodes the scripts to use scripts stored on this feature branch. For example:
After this PR has been tested and approved by reviewers, I will update the
curl
commands to usemain
before merging tomain
.Testing
Test installing and updating a project that uses the default application name
mkdir test-template-rails && cd test-template-rails
curl
from using previously cached versions of the script, usesthe installer in this branch, and installs a previous commit from the
main
branch.app-rails
.ls -la
should return this:cat .template-application-rails-version
should returna18946b606c258ce2e9a0f303741f1cca618c85e
.git diff
results:git init && git add -A . && git commit -m "Initial commit"
git diff
should return this:Test installing and updating a project that uses a custom application name
mkdir test-template-rails && cd test-template-rails
my-rails
.ls -la
should return this:ls -la .github/workflows
should return this:ls -la docs
should return this:cat .template-application-rails-version
should returna18946b606c258ce2e9a0f303741f1cca618c85e
.git diff
results:git init && git add -A . && git commit -m "Initial commit"
git diff
should return this:git status
should return this: