pipe-cd / pipecd

The One CD for All {applications, platforms, operations}
https://pipecd.dev
Apache License 2.0
1.08k stars 152 forks source link

[piped] Create a new pull request feature #4360

Open sivchari opened 1 year ago

sivchari commented 1 year ago

What would you like to be added:

Now, event-watcher of Piped's feature doesn't create a new pull request, when it pushes a new commit to repository. But a number of repository prohibits the push to default branch. So, I propose the new feature to create a pull request. This feature is optional, so if someone doesn't want this feature, they can ignore it. But if this idea is accepted, we will think a lot of things (e.g. About /pkg/git). So I want to discuss about this feature is needed or not.

Why is this needed:

nnnkkk7 commented 1 year ago

How about implementing it this way?

Overview Add an optional flag to Git in the following configuration to indicate whether or not to create a new branch. https://pipecd.dev/docs/user-guide/managing-piped/configuration-reference/#git

Details

If ok, I would like to create a new PR related with this. wdyt ?

sivchari commented 1 year ago

I agree with your opinion, but I would like to discuss if the field name is optimal with @nnnkkk7 and @khanhtc1202 . And do you think where it's better that uuid is generated ? I think uuid should be generated by a uuid-generator-client which piped-agent newly has. That why, Git client has no side-effect to test.

nnnkkk7 commented 1 year ago

@sivchari @khanhtc1202 Thanks! I think it is better from a testing point of view to create a uuid-generator-client, but currently other parts of the program also directly use google/uuid methods, so I would like to make it a separate scope for this Issue. And uuid-generator-client would make it easier to test other parts, so it would be good to set up such an issue.

nnnkkk7 commented 1 year ago

I created PR about this issue. https://github.com/pipe-cd/pipecd/pull/4395

khanhtc1202 commented 1 year ago

@nnnkkk7 @sivchari Thanks so much for your contribution πŸ™Œ Sorry for my late on design comments, the current implementation is acceptable. But there is a bit "UX kind" problem here I want to make it clear.

I think the git configuration (ref https://pipecd.dev/docs/user-guide/managing-piped/configuration-reference/#git) currently only focuses on configuring git auth to use the git command, not how the git command should be used.

If we enable this configuration in piped.spec.git, then at the time when users configure for their piped, they have to decide do they want that config or not, and they may don't even know about the event watcher. That's why I think we should move this configuration closed to event watcher related part, which should be

In case of using event watcher independent configuration (ref: https://pipecd.dev/docs/user-guide/event-watcher/#use-the-pipe-directory)

apiVersion: pipecd.dev/v1beta1
kind: EventWatcher
spec:
  events:
    - name: helloworld-image-update
      createMergeRequest: true
      replacements:
        - file: helloworld/deployment.yaml
          yamlField: $.spec.template.spec.containers[0].image

In case of using application configuration config

apiVersion: pipecd.dev/v1beta1
kind: KubernetesApp
spec:
  name: helloworld
  eventWatcher:
    - matcher:
        name: helloworld-image-update
      handler:
        type: GIT_UPDATE
        config:
          createMergeRequest: true
          replacements:
            - file: deployment.yaml
              yamlField: $.spec.template.spec.containers[0].image

wdyt?

khanhtc1202 commented 1 year ago

Also, the implementation at PR https://github.com/pipe-cd/pipecd/pull/4395 only commits the change in a new branch, I think we should make the merge request from that branch to the piped watching branch (main/master by default), or users can't complete their CI -> CD connection if they don't create the PR from the piped submitted branch manually.

nnnkkk7 commented 1 year ago

@khanhtc1202 Thanks so much!!!

That's why I think we should move this configuration closed to event watcher related part

I think so too. so, fix this in this commit . And I renamed flag. add createPullRequest flag to event watcher config instead of git config)

nnnkkk7 commented 1 year ago

Also, the implementation at PR https://github.com/pipe-cd/pipecd/pull/4395 only commits the change in a new branch, I think we should make the merge request from that branch to the piped watching branch (main/master by default)

Thanks! I added Push method after commit in push commit if the branch is new in event watcher PR, but should I add pullrequest method instead of just pushing? If so, would it be appropriate to create a CreatePullRequest method in pipecd/pkg/git/repo.go?

nnnkkk7 commented 1 year ago

Also, In the current implementation of pipecd/pkg/git/repo.go, we can't do more than use git command ,for example git push, git commit. To create a pullrequest, I think we need to replace it with an implementation like

  1. Use GitHub API
  2. Use GitHub API client Library like google/go-github

Would you have any other ideas?

khanhtc1202 commented 1 year ago

@nnnkkk7 Thanks for your investigation πŸ‘

You're right! The current implementation of the pipecd git package does not allow us to make pull requests 😒 We may have to move from using git cmd to using packages like google/go-github to make it programmable.

Let's review and merge the current createPullRequest configuration (aka. https://github.com/pipe-cd/pipecd/pull/4395) first, then create a separate one for automatically creating pull requests, since we have to change the implementation of the git package and it affects widely πŸ˜“

Btw, would you like to practice on that "move from git cmd to google/go-github" ? πŸ˜„

khanhtc1202 commented 1 year ago

Btw, I just realize that your 2 suggestions both only work with GitHub while pipecd needs to support overall git services (including Bitbucket, GitLab, etc) πŸ˜“ We have further investigation for the "auto-create pull request" feature πŸ€”

nnnkkk7 commented 1 year ago

@khanhtc1202 Thank you so much!!!

Let's review and merge the current createPullRequest configuration (aka. https://github.com/pipe-cd/pipecd/pull/4395) first, then create a separate one for automatically creating pull requests, since we have to change the implementation of the git package and it affects widely πŸ˜“

Ok! thank you for your review!

Btw, would you like to practice on that "move from git cmd to google/go-github" ? πŸ˜„

Yeah, I'm interested in it!:smile:

Btw, I just realize that your 2 suggestions both only work with GitHub while pipecd needs to support overall git services (including Bitbucket, GitLab, etc) πŸ˜“ We have further investigation for the "auto-create pull request" feature πŸ€”

I see. I didn't know about it. We need to investigate furtherπŸ€”

khanhtc1202 commented 1 year ago

@nnnkkk7 Thanks so much for the commitment πŸ™Œ I updated this issue and assigned you to handle this πŸ™

Let's investigate it, and leave any notes related here, thank you πŸ™Œ

github-actions[bot] commented 10 months ago

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

khanhtc1202 commented 10 months ago

Remove Stale label due to we would keep working on this

github-actions[bot] commented 4 months ago

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.