feat: flag to pull request create that allows update of existing PR #20

waciumawanjohi · 2022-05-05T03:49:51Z

Automated use of jx-scm may lead to repeated calls to create a PR between feature branch and main branch. If an existing PR exists between these two branches, the create call will fail. Users may prefer that the existing PR is simply updated. These changes enable that behavior by adding an option --allowUpdate flag.

The new tests rely on code in this PR in go-scm.

jenkins-x-bot · 2022-05-05T03:49:56Z

Hi @waciumawanjohi. Thanks for your PR.

I'm waiting for a jenkins-x-plugins or todo member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 jenkins-x/lighthouse repository.

waciumawanjohi · 2022-05-05T03:56:23Z

/assign @rawlingsj

waciumawanjohi · 2022-05-06T03:15:10Z

/retest

jenkins-x-bot · 2022-05-06T03:15:14Z

@waciumawanjohi: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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 jenkins-x/lighthouse repository.

rawlingsj · 2022-05-06T10:38:56Z

pkg/cmd/pull_request/create/create.go

@@ -75,6 +87,8 @@ func NewCmdCreatePullRequest() (*cobra.Command, *Options) {
 	cmd.Flags().StringVarP(&o.Head, "head", "", "", "the name of the branch where your changes are implemented")
 	cmd.Flags().StringVarP(&o.Base, "base", "", "main", "the name of the branch you want the changes pulled into")

+	cmd.Flags().BoolVarP(&o.AllowUpdate, "allowUpdate", "", false, "if an open pull request from head branch to base branch exists, setting flag to true will update the pull request")


Probably best to lowercase this flag, i.e. --allow-update so that it's consistent with other flags in jx CLI's

rawlingsj · 2022-05-06T10:39:42Z

All looks good, just the one comment to change to lowercase, then just regen the docs and we'll be good to go.

rawlingsj · 2022-05-06T11:56:05Z

/ok-to-test
/lgtm

jenkins-x-bot · 2022-05-06T11:56:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rawlingsj

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:

~~OWNERS~~ [rawlingsj]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ankitm123 · 2022-05-06T16:16:24Z

@waciumawanjohi rebase ur PR with the latest master/main, afte #22 is merged.

rawlingsj · 2022-05-06T16:22:57Z

/test this

ankitm123

Few comments ...

ankitm123 · 2022-05-06T16:18:22Z

pkg/cmd/pull_request/create/create_test.go

 	assert.Equal(t, prs[0].Title, o.Title, "title not properly set")
 	assert.Equal(t, prs[0].Body, o.Body, "body not properly set")
+}
+
+func createPRAgainWithoutAllowUpdate(t *testing.T, o *create_pr.Options, fullName string, scmClient *scm.Client, err error) {


may be use table driven tests instead?

ankitm123 · 2022-05-06T16:26:56Z

pkg/cmd/pull_request/create/create.go

+	if !updateAllowed {
+		return false, 0
+	}
+	pullRequestListOptions := scm.PullRequestListOptions{Page: 1, Size: 5000, Open: true, Closed: false}


Why 5000? Bit bucket for example sets the max at 100, so anything greater than that will result in bad request.
See (This was a workaround for bitbucket): https://github.com/jenkins-x/go-scm/pull/319/files#diff-d11bfd98e6b72330250844f3d1b2a7b94e88a4245870e9eb8bfcb58e2bef3918R48

I am not sure abt providers ...

Why 5000?
I wanted a value that could reasonably assure that it wouldn't miss open PRs, particularly as the use case for this feature is an automated system (which could create many pull requests).

Bit bucket for example sets the max at 100
That is great context on constraints. Will noodle.

Curious, should it be a configurable parameter from the cli? That way, users of different scm providers can set whichever value they want (according to the scm limits)

Curious, should it be a configurable parameter from the cli? That way, users of different scm providers can set whichever value they want (according to the scm limits)

I don't think any user would understand why they would pass a page and size flag to a command to create a pull request.

Indeed, this make me wonder if the go-scm API is correct here. As a consumer of go-scm I expect to receive a slice of pull request objects. I would understand passing a length for that list (e.g. "Please provide me the most recent 50 or 500 PRs"). But questions of page or size limits should be the concern of the code within go-scm. Should the PullRequestListOptions be changed?

Alternatively, should the provider specific code for listing PRs in go-scm handle page and size values of 0, setting them to reasonable defaults for the given provider?

ankitm123 · 2022-05-06T16:27:19Z

/hold
some minor comments ...

EDIT: (u can switch unit tests to table driven in a subsequent PR, just the 5000 bit is something I am unsure abt ...). Remove the hold after you have figured out if 5000 will break anything for other providers (including github/gitlab).

…pull request

Soothes the auto linting

jenkins-x-bot · 2022-05-06T16:29:55Z

New changes are detected. LGTM label has been removed.

waciumawanjohi · 2022-05-09T03:17:14Z

Remove the hold after you have figured out if 5000 will break anything for other providers (including github/gitlab).

I've been able to create a pull request and then update that pull request for repositories in both github and gitlab using the --allow-update flag. Is that sufficient? Or is there a list of SCMs that need manual testing?

ankitm123 · 2022-05-09T03:51:14Z

Or is there a list of SCMs that need manual testing

github and gitlab should be fine (we anyways test with only github atm with automated tests). It would be good to test with bitbucket cloud (did u check the limits), but we can look at that later.

waciumawanjohi · 2022-05-09T15:28:39Z

It would be good to test with bitbucket cloud (did u check the limits), but we can look at that later.

I've been trying to figure out how to work with bitbucket cloud at all. What is your workflow for generating a personal access token for bitbucket cloud?

Repository settings -> Access keys is about adding SSH keys
There are instructions for OAuth, but that doesn't seem applicable
I created an app password here, but using it returns error: failed to validate options: failed to validate options: failed to find current user: Not Authorized

ankitm123 · 2022-05-09T15:44:12Z

~~The version of go-scm in this project is ancient. There were a few fixes that went into go-scm, that fix a few issues with bitbucket cloud.~~
(U updated the version already, I forgot 🤦‍♂️ )
I also use app password when playing around with bitbucket cloud. Normally the interaction happens using lighthouse with go-scm. I have not used jx-scm before (did not even know this repo existed), but will give it a try with bitbucket cloud tonight, and update this thread. Can you send me the command you run to verify it with other scm providers?

waciumawanjohi · 2022-05-09T17:01:59Z

I decided to stop fighting the pagination and just use it. So the ridiculously large size value is gone.

One open question is how the different scm providers use that close value. E.g. Let's say that you have a repo where:

PR # 1 (least recent) is open.
PR # 2-# 11 are closed.
PR # 12 is open.

What will be the behavior when calling

scm.PullRequestListOptions{Page: 1, Size: 10, Open: true, Closed: false}

?

Will it return the 2 open PRs in the single page? Or will it return PR #12 and not PR #1 (because PR #1 is outside the pagination).

I tested manually against Github and it returns the 2 open PRs. But I don't know about the implementation of all other SCMs.

waciumawanjohi · 2022-05-09T17:03:17Z

My test command is:

./build/jx-scm pull-request create \
  --kind github \
  --server https://github.com/ \
  --token SOMETOKEN123 \
  --owner waciumawanjohi \
  --name demo-hello-world \
  --head hey \
  --title "It works, huzzah" \
  --body "Look here" \
  --base main \
  --allow-update

Run twice, it should succeed both times.

edit to add gitlab command:

./build/jx-scm pull-request create \
  --kind gitlab \
  --server https://gitlab.com/ \
  --token SOMETOKEN123 \
  --owner waciuma4work \
  --name demo-hello-world \
  --head hey \
  --title "It works, huzzah" \
  --body "Look here" \
  --base main \
  --allow-update

waciumawanjohi · 2022-05-10T06:28:10Z

This PR is blocked on this fix in go-scm:
jenkins-x/go-scm#328

sonarqubecloud · 2022-05-17T15:10:08Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.9% Duplication

waciumawanjohi · 2022-05-17T15:21:32Z

/hold cancel

jenkins-x-bot added needs-ok-to-test size/L labels May 5, 2022

jenkins-x-bot assigned rawlingsj May 5, 2022

rawlingsj reviewed May 6, 2022

View reviewed changes

jenkins-x-bot added lgtm ok-to-test and removed needs-ok-to-test labels May 6, 2022

jenkins-x-bot added the approved label May 6, 2022

ankitm123 reviewed May 6, 2022

View reviewed changes

jenkins-x-bot added the do-not-merge/hold label May 6, 2022

waciumawanjohi added 4 commits May 6, 2022 12:29

feat: add flag to pull request create that allows update of existing …

ad03f2b

…pull request

Refactor: Improve test, create docs

0bad37d

Update go-scm version used, dedup

7b4d5e6

Soothes the auto linting

rename flag to lowercase

ac69301

waciumawanjohi force-pushed the create-can-update branch from 08ab2d9 to ac69301 Compare May 6, 2022 16:29

jenkins-x-bot removed the lgtm label May 6, 2022

Leverage pagination for listing pull requests

8784461

Pass required flag in test

159c40e

Bump go-scm version

2c0ddd6

jenkins-x-bot removed the do-not-merge/hold label May 17, 2022

jenkins-x-bot merged commit a496e59 into jenkins-x-plugins:main May 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: flag to pull request create that allows update of existing PR #20

feat: flag to pull request create that allows update of existing PR #20

waciumawanjohi commented May 5, 2022

jenkins-x-bot commented May 5, 2022

waciumawanjohi commented May 5, 2022

waciumawanjohi commented May 6, 2022

jenkins-x-bot commented May 6, 2022

rawlingsj May 6, 2022

rawlingsj commented May 6, 2022

rawlingsj commented May 6, 2022

jenkins-x-bot commented May 6, 2022

ankitm123 commented May 6, 2022

rawlingsj commented May 6, 2022

ankitm123 left a comment

ankitm123 May 6, 2022

ankitm123 May 6, 2022 •

edited

Loading

waciumawanjohi May 6, 2022

ankitm123 May 9, 2022

waciumawanjohi May 9, 2022

ankitm123 commented May 6, 2022 •

edited

Loading

jenkins-x-bot commented May 6, 2022

waciumawanjohi commented May 9, 2022

ankitm123 commented May 9, 2022

waciumawanjohi commented May 9, 2022

ankitm123 commented May 9, 2022 •

edited

Loading

waciumawanjohi commented May 9, 2022 •

edited

Loading

waciumawanjohi commented May 9, 2022 •

edited

Loading

waciumawanjohi commented May 10, 2022

sonarqubecloud bot commented May 17, 2022

waciumawanjohi commented May 17, 2022

feat: flag to pull request create that allows update of existing PR #20

feat: flag to pull request create that allows update of existing PR #20

Conversation

waciumawanjohi commented May 5, 2022

jenkins-x-bot commented May 5, 2022

waciumawanjohi commented May 5, 2022

waciumawanjohi commented May 6, 2022

jenkins-x-bot commented May 6, 2022

rawlingsj May 6, 2022

Choose a reason for hiding this comment

rawlingsj commented May 6, 2022

rawlingsj commented May 6, 2022

jenkins-x-bot commented May 6, 2022

ankitm123 commented May 6, 2022

rawlingsj commented May 6, 2022

ankitm123 left a comment

Choose a reason for hiding this comment

ankitm123 May 6, 2022

Choose a reason for hiding this comment

ankitm123 May 6, 2022 • edited Loading

Choose a reason for hiding this comment

waciumawanjohi May 6, 2022

Choose a reason for hiding this comment

ankitm123 May 9, 2022

Choose a reason for hiding this comment

waciumawanjohi May 9, 2022

Choose a reason for hiding this comment

ankitm123 commented May 6, 2022 • edited Loading

jenkins-x-bot commented May 6, 2022

waciumawanjohi commented May 9, 2022

ankitm123 commented May 9, 2022

waciumawanjohi commented May 9, 2022

ankitm123 commented May 9, 2022 • edited Loading

waciumawanjohi commented May 9, 2022 • edited Loading

waciumawanjohi commented May 9, 2022 • edited Loading

waciumawanjohi commented May 10, 2022

sonarqubecloud bot commented May 17, 2022

waciumawanjohi commented May 17, 2022

ankitm123 May 6, 2022 •

edited

Loading

ankitm123 commented May 6, 2022 •

edited

Loading

ankitm123 commented May 9, 2022 •

edited

Loading

waciumawanjohi commented May 9, 2022 •

edited

Loading

waciumawanjohi commented May 9, 2022 •

edited

Loading