NO-ISSUE: Make target to automate releases #283
Conversation
|
@pkliczewski: This pull request explicitly references no jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
|
||
| $(DOCKER) tag docker-compose_workflow-service:latest $(ORG)$(WORKFLOW_SERVICE_IMAGE):$(GIT_BRANCH) | ||
| $(DOCKER) tag docker-compose_notification-service:latest $(ORG)$(NOTIFICATION_SERVICE_IMAGE):$(GIT_BRANCH) | ||
| $(DOCKER) tag docker-compose_workflow-service:latest $(ORG)$(WORKFLOW_SERVICE_IMAGE):$(TAG) |
There was a problem hiding this comment.
If this TAG we need to change in .github/workflows/
There was a problem hiding this comment.
The changes do not influence current behavior. Please see
$(eval TAG?=$ (GIT_HASH))
We still use hash and we only use TAG when we run release-all target
| all: mvn-checks java-checks install ## Build all modules | ||
|
|
||
| install: ARGS = $(FAST_BUILD_ARGS) | ||
| install: clean |
There was a problem hiding this comment.
As I see it make all is running maven install target
mvn-checks java-checks validation targets also should be done before make install
If you think it better, just can rename all to install?
There was a problem hiding this comment.
mvn install influenced the name of this target to be install. Running all in release sequence fails.
There was a problem hiding this comment.
So one of the mvn-checks or java-checks fails in the release sequence? why?
Makefile
Outdated
| bump-version: ## update post-release version and update commit message | ||
| find . -type f | xargs sed -i 's/$(RELEASE_VERSION)/${NEXT_VERSION}/g' | ||
|
|
||
| git-commit: ## adds all files and bumps the version |
There was a problem hiding this comment.
This is too general target name, need to have name that clarify it only for version bump
suggestion: bump-git-commit
Or maybe this target can be merged with bump-version target?
There was a problem hiding this comment.
Will change the name. Merging both targets is not good idea because they do not run one after the other.
Makefile
Outdated
|
|
||
| git-commit: ## adds all files and bumps the version | ||
| git add . | ||
| git commit -m 'Version bump' |
There was a problem hiding this comment.
Can we include also the version we bump to? (should ease the search when/if needed).
There was a problem hiding this comment.
Sure we can. I based the flow on current way of releasing. It makes sense to have version there.
There was a problem hiding this comment.
Overall lgtm, only one question: is it ok that the commit is unsigned?
Makefile
Outdated
| find . -type f | xargs sed -i 's/$(RELEASE_VERSION)/${NEXT_VERSION}/g' | ||
|
|
||
| git-commit: ## adds all files and bumps the version | ||
| git add . |
There was a problem hiding this comment.
May be a safer choice, use git add -u .? So it will commit only tracked files
There was a problem hiding this comment.
Good point, at this time we do not expect any files which are untracked. Will fix.
|
New changes are detected. LGTM label has been removed. |
New target `release-all` automates release process as well as builds, tags and pushes new version of images. Signed-off-by: Piotr Kliczewski <piotr.kliczewski@gmail.com>
| @@ -18,6 +18,8 @@ NOTIFICATION_SERVICE_IMAGE=notification-service | |||
|
|
|||
| # get version from pom | |||
| VERSION = $(shell sed -n "s/<revision>\(.*\)<\/revision>/\1/p" $(PWD)/pom.xml | sed 's/\ *//g') | |||
There was a problem hiding this comment.
I think it's safer to use something like 'mvn help:evaluate -Dexpression=project.version -q -DforceStdout' to get the project version. but if we can't use mvn cmd here then this is fine
There was a problem hiding this comment.
I will explore
There was a problem hiding this comment.
I could use mvn here but we would not be able to run make mvn-checks if maven not installed. Is there any other non maven command we could use here?
There was a problem hiding this comment.
I don't think so. let's keep sed then
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
New target
release-allautomates release process as well as builds, tags and pushes new version of images.Change type
Impacted services
Checklist