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

Create a new branch if makePullRequest is true in git config #4395

Merged
merged 18 commits into from
Feb 2, 2024

Conversation

nnnkkk7
Copy link
Contributor

@nnnkkk7 nnnkkk7 commented Jun 14, 2023

What this PR does / why we need it:
Add createPullRequest flag to event watcher config, to create new branch when commit changes in event watcher.

Which issue(s) this PR fixes:

Part of issue #4360

Does this PR introduce a user-facing change?:

Add makePullRequest option for users to choose whether or not to create a new branch when commit changes in event watcher.

Copy link
Member

@sivchari sivchari left a comment

Choose a reason for hiding this comment

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

Looks good to me. Some nits.

pkg/app/piped/eventwatcher/eventwatcher_test.go Outdated Show resolved Hide resolved
pkg/config/piped.go Outdated Show resolved Hide resolved
@nnnkkk7 nnnkkk7 force-pushed the add-new-branch-flag-to-git-conf branch from 59da8e7 to 84fa9b1 Compare June 23, 2023 01:54
@nnnkkk7 nnnkkk7 changed the title Create a new branch if enableNewBranch is true in git config Create a new branch if createPullRequest is true in git config Jun 23, 2023
@khanhtc1202
Copy link
Member

And besides, for simplicity, I think we can make this makePullRequest setting available only in app-config eventwatcher since the EventWatcher kind manifest is deprecated and will be removed later. There is no reason to add new features to deprecated configuration.

ref: https://pipecd.dev/docs/user-guide/event-watcher/#use-the-pipe-directory

@nnnkkk7 nnnkkk7 requested a review from khanhtc1202 June 26, 2023 06:57
@nnnkkk7 nnnkkk7 changed the title Create a new branch if createPullRequest is true in git config Create a new branch if makePullRequest is true in git config Jun 26, 2023
@nnnkkk7 nnnkkk7 requested a review from sivchari June 26, 2023 07:06
@nnnkkk7 nnnkkk7 force-pushed the add-new-branch-flag-to-git-conf branch from d2cffcc to 20222a3 Compare July 7, 2023 06:31
@nnnkkk7 nnnkkk7 requested a review from khanhtc1202 July 7, 2023 06:47
@nnnkkk7 nnnkkk7 force-pushed the add-new-branch-flag-to-git-conf branch from 20222a3 to c6ae27a Compare July 7, 2023 09:27
@nnnkkk7 nnnkkk7 requested a review from khanhtc1202 July 7, 2023 09:32
@nnnkkk7 nnnkkk7 requested a review from khanhtc1202 July 9, 2023 01:07
Signed-off-by: nnnkkk7 <[email protected]>
Signed-off-by: nnnkkk7 <[email protected]>
Signed-off-by: nnnkkk7 <[email protected]>
Signed-off-by: nnnkkk7 <[email protected]>
Signed-off-by: nnnkkk7 <[email protected]>
Signed-off-by: nnnkkk7 <[email protected]>
Copy link
Member

@ffjlabo ffjlabo left a comment

Choose a reason for hiding this comment

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

🚀 🚀

Copy link
Member

@khanhtc1202 khanhtc1202 left a comment

Choose a reason for hiding this comment

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

👍

@khanhtc1202 khanhtc1202 merged commit 82540c7 into pipe-cd:master Feb 2, 2024
11 checks passed
@ffjlabo
Copy link
Member

ffjlabo commented Feb 2, 2024

I'll check the behavior 👍

t-kikuc pushed a commit that referenced this pull request Feb 2, 2024
* add new branch flag when commit changes in eventwatcher

Signed-off-by: nnnkkk7 <[email protected]>

* add createPullRequest flag to event watcher config instead of git config

Signed-off-by: nnnkkk7 <[email protected]>

* fix test

Signed-off-by: nnnkkk7 <[email protected]>

* push commit if the branch is new in event watcher

Signed-off-by: nnnkkk7 <[email protected]>

* fix docs-v0.43.x

Signed-off-by: nnnkkk7 <[email protected]>

* fix docs

Signed-off-by: nnnkkk7 <[email protected]>

* replace createPullRequest with makePullRequest

Signed-off-by: nnnkkk7 <[email protected]>

* renamed getBranchName to makeBranchName

Signed-off-by: nnnkkk7 <[email protected]>

* remove makePullRequest from EventWatcherEvent

Signed-off-by: nnnkkk7 <[email protected]>

* retry push commits in execute func

Signed-off-by: nnnkkk7 <[email protected]>

* fix how to push new branch

Signed-off-by: nnnkkk7 <[email protected]>

* use map for branchname

Signed-off-by: nnnkkk7 <[email protected]>

* use branchHandledEvents map

Signed-off-by: nnnkkk7 <[email protected]>

* use errors.Join

Signed-off-by: nnnkkk7 <[email protected]>

* remove docs

Signed-off-by: nnnkkk7 <[email protected]>

* remove handledEvents

Signed-off-by: nnnkkk7 <[email protected]>

* avoid using '!'

Signed-off-by: nnnkkk7 <[email protected]>

* rearrange package

Signed-off-by: nnnkkk7 <[email protected]>

---------

Signed-off-by: nnnkkk7 <[email protected]>
@ffjlabo
Copy link
Member

ffjlabo commented Feb 2, 2024

makePullRequest: true

apiVersion: pipecd.dev/v1beta1
kind: KubernetesApp
spec:
  name: canary-with-script-run
...
  eventWatcher:
    - matcher:
        name: test
      handler:
        type: GIT_UPDATE
        config:
          makePullRequest: true
          replacements:
            - file: deployment.yaml
              yamlField: $.spec.template.spec.containers[0].image
  • eventName: test
pipectl event register \
    --address=XXX \
    --api-key=XXX \
    --name=test \
    --data=gcr.io/pipecd/helloworld:v0.45.0

✅ created the new branch named test-
ca-dp_ffjlabo-dev__dev_for_pipecd


✅ the change is as I expected!
Default__-zsh_

@ffjlabo
Copy link
Member

ffjlabo commented Feb 2, 2024

makePullRequest: false

apiVersion: pipecd.dev/v1beta1
kind: KubernetesApp
spec:
  name: canary-with-script-run
...
  eventWatcher:
    - matcher:
        name: test
      handler:
        type: GIT_UPDATE
        config:
          makePullRequest: false
          replacements:
            - file: deployment.yaml
              yamlField: $.spec.template.spec.containers[0].image
  • eventName: test
pipectl event register \
    --address=XXX \
    --api-key=XXX \
    --name=test \
    --data=gcr.io/pipecd/helloworld:v0.45.0

✅ pushed the changes directly
Commits_·_ca-dp_ffjlabo-dev

✅ the change is as I expected!
Replace_values_with__gcr_io_pipecd_helloworld_v0_45_0__set_by_Event__…_·_ca-dp_ffjlabo-dev_18b75ec

@ffjlabo
Copy link
Member

ffjlabo commented Feb 2, 2024

not set makePullRequest

apiVersion: pipecd.dev/v1beta1
kind: KubernetesApp
spec:
  name: canary-with-script-run
...
  eventWatcher:
    - matcher:
        name: test
      handler:
        type: GIT_UPDATE
        config:
          replacements:
            - file: deployment.yaml
              yamlField: $.spec.template.spec.containers[0].image
pipectl event register \
    --address=XXX \
    --api-key=XXX \
    --name=test \
    --data=gcr.io/pipecd/helloworld:v0.44.0

✅ pushed the changes directly
Commits_·_ca-dp_ffjlabo-dev

✅ the change is as I expected!
Replace_values_with__gcr_io_pipecd_helloworld_v0_44_0__set_by_Event__…_·_ca-dp_ffjlabo-dev_e644d49

@ffjlabo
Copy link
Member

ffjlabo commented Feb 2, 2024

It works fine! Thank you! @nnnkkk7

@ffjlabo
Copy link
Member

ffjlabo commented Feb 2, 2024

Multi events (one is true, other is false)

apiVersion: pipecd.dev/v1beta1
kind: KubernetesApp
spec:
  name: canary-with-script-run
...
  eventWatcher:
    - matcher:
        name: test
      handler:
        type: GIT_UPDATE
        config:
          makePullRequest: false
          replacements:
            - file: deployment.yaml
              yamlField: $.spec.template.spec.containers[0].image
    - matcher:
        name: hoge
      handler:
        type: GIT_UPDATE
        config:
          makePullRequest: true
          replacements:
            - file: deployment.yaml
              yamlField: $.spec.replicas

test: makePullRequest false
hoge: makePullRequest true

✅ Created a new branch when the event hoge happens

pipectl event register \
    --address=XXX \
    --api-key=XXX \
    --name=hoge \
    --data=1
Commits_·_ca-dp_ffjlabo-dev

The change is as I expected 👍
Comparing_main___hoge-66fc0893-636c-4fbd-ad94-3ebc4ab824d5_·_ca-dp_ffjlabo-dev

✅ Pushed the changes directly to the main branch when the event test happens

pipectl event register \
    --address=XXX \
    --api-key=XXX \
    --name=test \
    --data=gcr.io/pipecd/helloworld:v0.45.0
Commits_·_ca-dp_ffjlabo-dev Replace_values_with__gcr_io_pipecd_helloworld_v0_45_0__set_by_Event__…_·_ca-dp_ffjlabo-dev_e0ffa57

@ffjlabo
Copy link
Member

ffjlabo commented Feb 2, 2024

Multi events (all are true)

apiVersion: pipecd.dev/v1beta1
kind: KubernetesApp
spec:
  name: canary-with-script-run
...
  eventWatcher:
    - matcher:
        name: test
      handler:
        type: GIT_UPDATE
        config:
          makePullRequest: true
          replacements:
            - file: deployment.yaml
              yamlField: $.spec.template.spec.containers[0].image
    - matcher:
        name: hoge
      handler:
        type: GIT_UPDATE
        config:
          makePullRequest: true
          replacements:
            - file: deployment.yaml
              yamlField: $.spec.replicas

test: makePullRequest true
hoge: makePullRequest true

for event test

pipectl event register \
    --address=XXX \
    --api-key=XXX \
    --name=test \
    --data=gcr.io/pipecd/helloworld:v0.44.0

for event hoge

pipectl event register \
    --address=XXX \
    --api-key=XXX \
    --name=hoge \
    --data=1


✅ Created a new branch when the event test happens and also for the event hoge

ca-dp_ffjlabo-dev__dev_for_pipecd

✅ These changes are also OK.
Comparing_main___test-3bd27464-44a2-4401-987e-435fbbc010fa_·_ca-dp_ffjlabo-dev

Comparing_main___hoge-fb0aaf10-5dd2-4b53-8fbf-ea9fd0bd06f6_·_ca-dp_ffjlabo-dev

@github-actions github-actions bot mentioned this pull request Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants