scylladb / scylla-bench

43 stars 36 forks source link

Introduce Makefile to build image with custom gocql tag #144

Closed dkropachev closed 2 months ago

sylwiaszunejko commented 2 months ago

@dkropachev Why new PR was necessary? If one that I made was not in line with your expectations I would address the comments. And why merging PR without the review process? Is this considered small enough change to do that?

sylwiaszunejko commented 2 months ago

There is a problem with this Makefile: https://github.com/scylladb/gocql/actions/runs/10884803874/job/30200923392?pr=263 error: No rule to make target '_use-custom-gocql-tag', needed by 'build-with-custom-gocql-version'. Stop.

dkropachev commented 2 months ago

There is a problem with this Makefile: https://github.com/scylladb/gocql/actions/runs/10884803874/job/30200923392?pr=263 error: No rule to make target '_use-custom-gocql-tag', needed by 'build-with-custom-gocql-version'. Stop.

Fixed

sylwiaszunejko commented 2 months ago

It is still not working: https://github.com/scylladb/gocql/actions/runs/10884803874/job/30201321291?pr=263 maybe I am using it wrong:

GOCQL_REPO="${{ github.event.pull_request.head.repo.full_name }}" GOCQL_COMMIT_ID="${{ github.event.pull_request.head.ref }}" make build-with-custom-gocql-version
DOCKER_IMAGE_TAG="scylladb/gocql-extended-ci:scylla-bench-${{ github.event.pull_request.head.ref }}" make build-docker-image
dkropachev commented 2 months ago

It is still not working: https://github.com/scylladb/gocql/actions/runs/10884803874/job/30201321291?pr=263 maybe I am using it wrong:

GOCQL_REPO="${{ github.event.pull_request.head.repo.full_name }}" GOCQL_COMMIT_ID="${{ github.event.pull_request.head.ref }}" make build-with-custom-gocql-version
DOCKER_IMAGE_TAG="scylladb/gocql-extended-ci:scylla-bench-${{ github.event.pull_request.head.ref }}" make build-docker-image

This one should work:

GOCQL_REPO="${{ github.event.pull_request.head.repo.full_name }}" GOCQL_VERSION="${{ github.event.pull_request.head.ref }}" make build-with-custom-gocql-version
DOCKER_IMAGE_TAG="scylladb/gocql-extended-ci:scylla-bench-${{ github.event.pull_request.head.ref }}" make build-docker-image
vponomaryov commented 2 months ago

@dkropachev Why new PR was necessary? If one that I made was not in line with your expectations I would address the comments. And why merging PR without the review process? Is this considered small enough change to do that?

I agree. This force-merge was not required to be "force" and led to a need to create hot-fixes which then got pushed directly to the branch.

We should avoid such processes and stick to general standard - review [+ test-try-out].

dkropachev commented 2 months ago

@dkropachev Why new PR was necessary? If one that I made was not in line with your expectations I would address the comments. And why merging PR without the review process? Is this considered small enough change to do that?

I agree. This force-merge was not required to be "force" and led to a need to create hot-fixes which then got pushed directly to the branch.

We should avoid such processes and stick to general standard - review [+ test-try-out].

I agree on all these points, following played role in why I did that in the order of priority:

  1. There was no Makefile before, and it does not interfere into any process/code that is in the repo.
  2. To fix it in original PR would make spend at least two hours on both sides, writing back and forth, laying out Makefile directory structure, .gitignore, how exactly use Dockerfile etc...
  3. On other hand quickly submit PR and merge it took 15 minutes at max, it did not go as smooth as could be afterwards, but even at this point it did not take 5 minutes on top of original 15 minutes for me, I and guess it also was not that time consuming on your end as well,