-
Notifications
You must be signed in to change notification settings - Fork 2
Automate make assets #38
base: master
Are you sure you want to change the base?
Conversation
scripts/tag_release_manually.sh
Outdated
make assets | ||
|
||
echo -e "${GREEN}Commit assets >> git commit -m 'fix: re-generate assets'${NC}" | ||
git add pkg/template/assets_vfsdata.go |
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.
You probably need to check that make assets
actually modified anything…
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.
Actually, maybe it would be better to just check if there are any changes due to make assets
and exit the script if they are to let the user handle things because automatic commit and push from git is dangerous (for example, what happens if there is no origin
master? Or if origin
doesn't point to what we think it does?) 😄
What do you think @aureamunoz?
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.
Yes, true. Less cases to take in account for something that is not hard to do manually
RELEASE.md
Outdated
@@ -6,15 +6,15 @@ Simply create a release using the script `./script/tag_release_manually.sh` wher | |||
`ID` of the release to be created : | |||
|
|||
```bash | |||
./script/tag_release_manually.sh GITHUB_API_TOKEN VERSION | |||
./scripts/tag_release_manually.sh GITHUB_API_TOKEN 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.
Have you checked if other code is not impacted by that change? I would have rather preferred that change in a different PR since it's not really related to what you're trying to accomplish here…
So, what do we do then ? |
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.
Why are assets changed in this PR?
scripts/tag_release_manually.sh
Outdated
then | ||
DIFF_FILES="$(git diff-files --ignore-submodules)" | ||
if [[ $DIFF_FILES == *"pkg/template/assets_vfsdata.go"* ]]; then | ||
echo >&2 "cannot $1: you have assets unstaged changes." |
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.
What is $1
in this context?
… for checking if there are assets to commit/push on origin master
ae932b4
to
ef38d44
Compare
I've tried to simplify and clean the function doing the check about assets and add a comment in the doc to be clearer. WDYT @metacosm ? |
As we talked I have add a task for doing the
make assets
and commit/push the files during the release