WIP: feat: use bumpp to increase version #2929

Closed
dpschen wants to merge 3 commits from dpschen/frontend:feature/use-bumpp-for-version into main
Member

Related backend PR: vikunja/api#1472

Related backend PR: https://kolaente.dev/vikunja/api/pulls/1472
dpschen added the
kind/feature
area/internal-code
labels 2023-01-05 18:46:53 +00:00
konrad was assigned by dpschen 2023-01-05 18:46:53 +00:00
dpschen reviewed 2023-01-05 21:54:32 +00:00
.drone.yml Outdated
@ -108,3 +108,3 @@
from_secret: cypress_project_key
commands:
- sed -i 's/localhost/api/g' dist/index.html
- sed -i '' 's|localhost|api|g' dist/index.html
Author
Member

I use | as delimiter now consequently for sed since we replace a lot of urls, where | reduces the need to escape. The leading '' is not necessary here but makes it possible to copy these commands and run them on mac.

I use `|` as delimiter now consequently for sed since we replace a lot of urls, where `|` reduces the need to escape. The leading `''` is not necessary here but makes it possible to copy these commands and run them on mac.
dpschen marked this conversation as resolved
.drone.yml Outdated
@ -148,3 +148,3 @@
- cp -r dist dist-preview
# Override the default api url used for preview
- sed -i 's|http://localhost:3456|https://try.vikunja.io|g' dist-preview/index.html
- sed -i '' 's|http\://localhost\:3456|https\://try\.vikunja\.io|g' dist-preview/index.html
Author
Member

I read that . and : on the other hand need to be escaped. Probably I removed the escaping here at some other time. Wrong!

~~I read that `.` and `:` on the other hand need to be escaped. Probably I removed the escaping here at some other time.~~ Wrong!
dpschen marked this conversation as resolved
package.json Outdated
@ -22,1 +22,3 @@
"fonts:subset": "./scripts/fonts-subset.sh"
"fonts:subset": "./scripts/fonts-subset.sh",
"// TODO:": "add release including creating changelog via `git-cliff`",
"bump-version": "pnpm dlx bumpp --no-push \"$(bash ./scripts/update-package-version.sh)\""
Author
Member

We should also include the git-cliff changelog creation script.

We should also include the git-cliff changelog creation script.

That's pretty much git cliff <last version tag>..HEAD (not sure how we could always get the last tag). The output then needs to manually copied into the changelog.md file and in the tag message.

That's pretty much `git cliff <last version tag>..HEAD` (not sure how we could always get the last tag). The output then needs to manually copied into the `changelog.md` file and in the tag message.

This seems to work:

git cliff `git tag | sort -V | tail -n1`..HEAD
This seems to work: ``` git cliff `git tag | sort -V | tail -n1`..HEAD ```
Author
Member

Shouldn't we better use a template?

The output then needs to manually copied into the changelog.md file and in the tag message.

Shouldn't we better use a template? > The output then needs to manually copied into the changelog.md file and in the tag message.
Author
Member
@konrad

You mean in git-cliff? Not 100% sure how we would do that. iirc git-cliff can generate a full changelog (not just the changes since the last release) but that does not work when the old commits do not use semantic commits.

You mean in git-cliff? Not 100% sure how we would do that. iirc git-cliff can generate a full changelog (not just the changes since the last release) but that does not work when the old commits do not use semantic commits.
scripts/run.sh Outdated
@ -12,3 +12,4 @@
# I think this is not necessary since I changed the delimiters to '|'
# Escape the variable to prevent sed from complaining
VIKUNJA_API_URL=$(echo $VIKUNJA_API_URL |sed 's/\//\\\//g')
Author
Member

Do you think we still need this?

Do you think we still need this?

Probably not, can we test this?

Probably not, can we test this?
Author
Member

Removed.

  • Tested
Removed. - [ ] Tested
@ -0,0 +25,4 @@
git describe --tags --always --abbrev=10 | sed 's/^v//' | sed 's/-g/-/'
}
# # Update the version field in package.json
Author
Member

I kept the following part in case we don't want to use bumpp

I kept the following part in case we don't want to use bumpp
dpschen marked this conversation as resolved
vite.config.ts Outdated
@ -37,2 +38,4 @@
export default defineConfig({
// https://vitest.dev/config/
define: {
'__APP_VERSION__': version,
Author
Member

By using define for the __APP_VERSION__ it gets replaced in the whole app.

By using `define` for the `__APP_VERSION__` it gets replaced in the whole app.
dpschen marked this conversation as resolved
dpschen requested review from konrad 2023-01-05 21:54:49 +00:00
dpschen reviewed 2023-01-05 22:03:19 +00:00
Dockerfile Outdated
@ -22,4 +22,2 @@
# Build the frontend
pnpm install && \
apk add --no-cache git && \
echo '{"VERSION": "'$(git describe --tags --always --abbrev=10 | sed 's/-/+/' | sed 's/^v//' | sed 's/-g/-/')'"}' > src/version.json && \
Author
Member

I removed the part where sed 's/-/+/' happens.
Was there a specific reason why we replace the - with a +?
I needed to remove it because else the resulting string wouldn't be compatible with semver.

I removed the part where `sed 's/-/+/'` happens. Was there a specific reason why we replace the `-` with a `+`? I needed to remove it because else the resulting string wouldn't be compatible with semver.

The string contains the git commit for unstable builds plus the number of commits. I felt like this was a nicer option but it's eye-candy only so fine to remove.

The string contains the git commit for unstable builds plus the number of commits. I felt like this was a nicer option but it's eye-candy only so fine to remove.
Author
Member

I keep the string. I only changed the part where - was changed to +

I keep the string. I only changed the part where `-` was changed to `+`

That's fine. Again, was only eyes-candy before.

That's fine. Again, was only eyes-candy before.
dpschen marked this conversation as resolved
dpschen force-pushed feature/use-bumpp-for-version from ba607b2afc to 5418bfcfd6 2023-01-05 22:06:36 +00:00 Compare
dpschen force-pushed feature/use-bumpp-for-version from 5418bfcfd6 to f7d8ffdf0e 2023-01-05 22:08:09 +00:00 Compare
konrad reviewed 2023-01-06 10:02:04 +00:00
.drone.yml Outdated
@ -207,3 +206,3 @@
- pnpm install --fetch-timeout 100000 --frozen-lockfile
- pnpm run lint
- "echo '{\"VERSION\": \"'$(git describe --tags --always --abbrev=10 | sed 's/-/+/' | sed 's/^v//' | sed 's/-g/-/')'\"}' > src/version.json"
- source ./scripts/update-package-version.sh

Why not just call the script? (Without the source)

Why not just call the script? (Without the `source`)
dpschen marked this conversation as resolved
@ -0,0 +1,31 @@
#!/bin/bash
# ensure packages are installed
if [ "$(uname)" == "Darwin" ]; then

I think we should rather check if the tools are installed and if not exit 1 the script and prompt the user to install them. There are too many OS variants which we cannot cover. Also I think it's a bit unexpected for a version update script to install stuff.

We should also add these to the nix flake definition.

I think we should rather check if the tools are installed and if not exit 1 the script and prompt the user to install them. There are too many OS variants which we cannot cover. Also I think it's a bit unexpected for a version update script to install stuff. We should also add these to the nix flake definition.
Author
Member

Also I think it's a bit unexpected for a version update script to install stuff.

that would then also apply to fonttools? I have no experience with nix so I didn't know if or where I should add this.

> Also I think it's a bit unexpected for a version update script to install stuff. that would then also apply to fonttools? I have no experience with nix so I didn't know if or where I should add this.

The flake definition is here: https://kolaente.dev/vikunja/frontend/src/branch/main/flake.nix#L8
The jq package is called simply jq and thus it should be enough to add an entry pkgs.jq next to the other packages.

Ideally this would apply to fronttools as well, yes.

The flake definition is here: https://kolaente.dev/vikunja/frontend/src/branch/main/flake.nix#L8 The `jq` package is called simply `jq` and thus it should be enough to add an entry `pkgs.jq` next to the other packages. Ideally this would apply to fronttools as well, yes.
Author
Member

What about git, docker, wget, b2sum, awk, python (for fonttools) and brotli (for woff2 compression).

What about git, docker, wget, b2sum, awk, python (for fonttools) and brotli (for woff2 compression).

Probably a good idea to add those as well. Hit is implicit as it is required for flakes to work.

Probably a good idea to add those as well. Hit is implicit as it is required for flakes to work.
Author
Member

I don't have enough knowledge of nixos to modify the flake. I tried to install the manager but it failed and I had to revert (it's really complex on mac: if I got it right they create an extra partition). Can you help me here?

I don't have enough knowledge of nixos to modify the flake. I tried to install the manager but it failed and I had to revert (it's really complex on mac: if I got it right they create an extra partition). Can you help me here?

Not sure how this works on mac but as per their docs it's correct the installer tries to create an extra partition 🤔

I can add the extra dependencies to the flake if that makes it easier.

Not sure how this works on mac but as per their docs it's correct the installer tries to create an extra partition 🤔 I can add the extra dependencies to the flake if that makes it easier.

Flake file seems to work now.

Flake file seems to work now.

Did you try with the installer here? https://nixos.org/download.html#nix-install-macos

Did you try with the installer here? https://nixos.org/download.html#nix-install-macos
Author
Member

Yes, that's how I tried.

Yes, that's how I tried.
Author
Member

@konrad And these are already included?

What about git, wget, b2sum, awk, python (for fonttools) and brotli (for woff2 compression).

@konrad And these are already included? > What about git, wget, b2sum, awk, ~~python (for fonttools)~~ and brotli (for woff2 compression).

Now they are all added.

Now they are all added.
Author
Member
Unsure: are the [python bindings for brotli](https://pypi.org/project/Brotli/) included? See: https://fonttools.readthedocs.io/en/latest/optional.html#fonttools-ttlib-woff2
Looks like it: https://github.com/NixOS/nixpkgs/blob/nixos-22.11/pkgs/development/python-modules/fonttools/default.nix#L44
Author
Member

Nice, I guess this is solved then :)

Nice, I guess this is solved then :)
dpschen marked this conversation as resolved
@ -0,0 +9,4 @@
# Alpine
apk add --no-cache git jq
else
apk-get install git jq

This should be apt-get (Assuming you meant to install on debian-like systems)

This should be `apt-get` (Assuming you meant to install on debian-like systems)
Author
Member

Ups. I know, was only confused because of alpines apk.

Ups. I know, was only confused because of alpines `apk`.
dpschen marked this conversation as resolved
@ -0,0 +26,4 @@
# # Update the version field in package.json
# new_version=$(get_new_version_string)
# jq '.version = "'$new_version'"' package.json > package.json.tmp

Why is this commented?

Why is this commented?
Author
Member

Separated this part in two scripts now, because I needed them in the in the package.json and in the drone config.

Separated this part in two scripts now, because I needed them in the in the package.json and in the drone config.
dpschen marked this conversation as resolved
Owner

Can we document the required steps to publish a new release?

Can we document the required steps to publish a new release?
dpschen force-pushed feature/use-bumpp-for-version from f7d8ffdf0e to bb6ac732da 2023-01-06 13:05:23 +00:00 Compare
Author
Member

Can we document the required steps to publish a new release?

I can update https://vikunja.io/docs/releasing-a-new-vikunja-version/
Is that what you mean?

> Can we document the required steps to publish a new release? I can update https://vikunja.io/docs/releasing-a-new-vikunja-version/ Is that what you mean?
Owner

Can we document the required steps to publish a new release?

I can update https://vikunja.io/docs/releasing-a-new-vikunja-version/
Is that what you mean?

yes.

> > Can we document the required steps to publish a new release? > > I can update https://vikunja.io/docs/releasing-a-new-vikunja-version/ > Is that what you mean? yes.
dpschen force-pushed feature/use-bumpp-for-version from bb6ac732da to 73edb57fe8 2023-01-06 16:21:12 +00:00 Compare
dpschen force-pushed feature/use-bumpp-for-version from 73edb57fe8 to 2cad2c8f7d 2023-01-06 16:46:41 +00:00 Compare
konrad reviewed 2023-01-06 21:45:23 +00:00
@ -0,0 +1,26 @@
#!/bin/bash

This should be /usr/bin/env bash or /bin/sh (not sure if we're using any bash specific things here)

This should be `/usr/bin/env bash` or `/bin/sh` (not sure if we're using any bash specific things here)
Author
Member

Shouldn' we then not also update scripts/run.sh?

Shouldn' we then not also update `scripts/run.sh`?

Won't hurt. The main difference is the run script only gets executed in the docker container whereas this is meant to run on dev computers.

Won't hurt. The main difference is the run script only gets executed in the docker container whereas this is meant to run on dev computers.
Author
Member

Will update in order to align.

Will update in order to align.
dpschen marked this conversation as resolved
konrad reviewed 2023-01-06 21:45:53 +00:00
@ -0,0 +1,5 @@
get_version_string() {

Doesn't this script miss a shebang comment at the top?

Doesn't this script miss a shebang comment at the top?
dpschen marked this conversation as resolved
konrad reviewed 2023-01-06 21:46:21 +00:00
vite.config.ts Outdated
@ -4,6 +4,7 @@ import vue from '@vitejs/plugin-vue'
import legacyFn from '@vitejs/plugin-legacy'
import { URL, fileURLToPath } from 'node:url'
import { dirname, resolve } from 'node:path'
import {version} from './package.json'

Didn't know you could do that, very nice!

Didn't know you could do that, very nice!
Author
Member

I think it’s a vite feature. but did already work with webpack.

I think it’s a vite feature. but did already work with webpack.
dpschen marked this conversation as resolved
dpschen force-pushed feature/use-bumpp-for-version from 2cad2c8f7d to 9d7ffc30bd 2023-01-17 16:09:15 +00:00 Compare
konrad added 1 commit 2023-01-21 11:18:30 +00:00
continuous-integration/drone/pr Build is pending Details
e682724a13
fix(flake): add correct fonttools package
konrad added 1 commit 2023-01-24 22:14:14 +00:00
continuous-integration/drone/pr Build is pending Details
79dd8638ee
chore: add missing tools to nix flake
Author
Member

@konrad sadly this broke now.
I'm not sure how to untangle this without breaking some of the new docker code TBH.

@konrad sadly this broke now. I'm not sure how to untangle this without breaking some of the new docker code TBH.
Owner

@dpschen mhh yeah I see what you mean

@dpschen mhh yeah I see what you mean
Owner

Is the main problem with untangling it in the Dockerfile and run.sh?

Is the main problem with untangling it in the Dockerfile and `run.sh`?
Author
Member

Is the main problem with untangling it in the Dockerfile and run.sh?

I didn't check the drone config, but those two seem to be the ones that have the biggest complexity.

> Is the main problem with untangling it in the Dockerfile and `run.sh`? I didn't check the drone config, but those two seem to be the ones that have the biggest complexity.
Owner

Closing this as it appears stale.

Please rebase and reopen if you think this is still relevant.

Closing this as it appears stale. Please rebase and reopen if you think this is still relevant.
konrad closed this pull request 2023-10-22 17:30:02 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.