New CLI option: --run-make to run make and commit changes#24
New CLI option: --run-make to run make and commit changes#24openshift-merge-bot[bot] merged 2 commits intomainfrom
--run-make to run make and commit changes#24Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: EmilienM The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/assign pierreprinetti |
|
/test |
|
@EmilienM: The
Use DetailsIn 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-sigs/prow repository. |
|
/test cluster-api-provider-openstack-main |
|
@EmilienM: The specified target(s) for
Use DetailsIn 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-sigs/prow repository. |
| logging.debug(f"make merge-bot output: {proc.stdout.decode()}") | ||
| except subprocess.CalledProcessError as err: | ||
| if "No rule to make target" not in err.stderr.decode(): | ||
| raise RepoException( |
There was a problem hiding this comment.
Should we really raise a RepoException when the repository doesn't have a make merge-bot target, and trigger a Manual intervention is needed to merge {source} into {dest} message on slack? IMO, we should log what happens but not send update to slack, so perhaps raise a different exception type.
There was a problem hiding this comment.
that was a bad copy paste.
There was a problem hiding this comment.
- If the repo doesn't have
merge-bottarget, just move on: no error at all. - If the repo has the target but running it failed, we send an error but we also send an update on Slack saying that manual intervention is needed in the PR. I think it's fine this way.
| if update_go_modules: | ||
| commit_go_mod_updates(gitwd) | ||
|
|
||
| run_make(gitwd) |
There was a problem hiding this comment.
Shouldn't we more this under the if update_go_modules: conditional just above? Also it feels like we want run_make() to run before commit_go_mod_updates()
Perhaps this would be good to split the functions, and have them only generate updated content, then in a separate function commit the new content if there is anything to commit.
There was a problem hiding this comment.
We have to run make after go mod is updated in order to update all assets after go vendoring was updated.
Also I don't want to add the make block within the go block since any repo could have this Makefile target, so let's make them separate in that order.
There was a problem hiding this comment.
Please move run_make(gitwd) in the if update_go_modules: branch. This ensures we don't accidentally create commits unless the user wants us to re-generate code and vendoring.
https://github.com/shiftstack/merge-bot/blob/main/src/merge_bot/cli.py#L164-L169
There was a problem hiding this comment.
I think the run_make is not only for go projects. Any project could have a merge-bot target they want to run when merging. Isn't?
There was a problem hiding this comment.
Then make a new option. I believe the intention is that by default we don't add more commits that the one we merge from the upstream repository. @mdbooth @pierreprinetti, any thoughts?
There was a problem hiding this comment.
I'm happy with a new function. I'll wait for Pierre & Matt in case they have any feedback otherwise I'll propose it by end of week.
There was a problem hiding this comment.
I've done the change Martin.
EmilienM
force-pushed
the
makefile
branch
3 times, most recently
from
71acefe to
28b9d30
Compare
EmilienM
changed the title
make as a new hook--run-make to run make and commit changes
Some projects need to run custom tasks when merging with upstream, like vendoring or code generation. Let's try to run `make merge-bot` If the target doesn't exist we return an error. Example on where it'll be used: openshift/cluster-api-provider-openstack#332 This feature can be used through a new CLI option: `--run-make`.
* use Fedora * install make, git, difftools so we can run make and dependencies needed in the future to run make in our projects.
3 participants
Some projects need to run custom tasks when merging with upstream, like
vendoring or code generation. Let's try to run
make merge-botIf the target doesn't exist we return an error.
Example on where it'll be used: openshift/cluster-api-provider-openstack#332
This feature can be used through a new CLI option:
--run-make.