Skip to content
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

Feature/release #123

Open
wants to merge 25 commits into
base: feature/documentation
Choose a base branch
from
Open

Feature/release #123

wants to merge 25 commits into from

Conversation

CSchoel
Copy link
Contributor

@CSchoel CSchoel commented Aug 13, 2021

I created this pull request prematurely in order to be able to provide a comprehensive code review. This branch is still work in progress.

@CSchoel CSchoel changed the base branch from dev to feature/documentation August 13, 2021 09:02
@CSchoel
Copy link
Contributor Author

CSchoel commented Aug 13, 2021

I changed the base of the branch to feature/documentation so that I do not see the changes related to the project architecture documentation in this pull request.

Copy link
Contributor Author

@CSchoel CSchoel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK for the most part, just a few smaller comments.

- name: Test with Gradle
run: ./gradlew test -i
- name: Build with Gradle
run: ./gradlew build -i
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why build and not test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change means we will not get test results and code coverage anymore for each commit.

Comment on lines +51 to +52
- name: Grant execute permission for gradlew
run: chmod +x gradlew
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've said this before, but this should not be necessary if the file is checked into git with the correct permissions. Git is aware of file permissions.

run: ./gradlew shadowClientJar shadowServerJar
- name: Add version from build.gradle to output
id: version
run: echo "::set-output name=version::$(./gradlew getVersion -q)"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of getting the version from Gradle, but you didn't have to write a separate gradle task for this. This line of bash code would also do the trick:

./gradlew properties | grep version | sed "s/version: //"

Grep selects the line containing the version, the sed command replaces the string "version: " that is in front of the actual version number with the empty string.

Comment on lines +68 to +69
draft: false
prerelease: true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest doing this the other way around. A prerelease is for hotfixes and stuff that has to get out there before the "official" release is done. I think this script is more for generating regular releases, but I would set the draft state to true, so that you can review your release manually before it is made public to all users of the software.

target_commitish: ${{github.sha}} #Get latest commit on this branch
draft: false
prerelease: true
tag_name: v${{ steps.version.outputs.version }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for your setup, but is not fully correct. If you name your next tag v0.1.0 instead of 0.1.0, this would fail. Instead, you should use github.ref and remove the trailing refs/tag/ from the full ref, e.g. by using the parameter expansion of the shell like this: ${GITHUB_REF##*/}. In this case, all trailing patterns of the form */ found in the variable $GITHUB_REF are removed, leaving just the tag name (or any other remaining part of the ref that is not followed by a slash).

Comment on lines +73 to +75
body: |
Changes in this Release
- separating build and release
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sure you are aware of that, but this should be replaced by a dynamic script generating the release body from the change log or any other source on the file system. Hard-coding the release notes in the Gradle script is a little clunky.

@@ -4,7 +4,7 @@ plugins {
}

group 'thm.mope.lsp'
version '0.1.0'
version '0.0.9-test3'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small note, but be careful with downgrading version numbers. This will confuse any package manager, because the assumptions with version numbers is always that higher versions are more up to date than lower ones. Some package managers therefore only allow you to publish versions of your software that are a direct upgrade from an already existing version. So if you have, for example 0.0.9 and 0.1.0, you can upload versions 0.0.10, 0.1.1, 0.2.0, or 1.0.0, but these are your only options. There is no harm in already sticking to this scheme during initial development.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants