Don't close forks, don't close PRs for child/parent commands #142

justinharringa · 2020-04-17T04:20:20Z

It has proven wasteful and confusing to remove forks.

This changes the parent and child commands to use branches with a name based on the new image-tag combination.

This will have the following benefits:

Stop closing forks and creating new ones
- This will keep PRs for other image-tag combos open
Use filename:Dockerfile instead of language:Dockerfile which picks up more results
Create a branch specific to the image-tag combo we're updating to (which provides more context for the PR)
- A benefit here is that it is also easier to locate DFIU PRs without requiring the usage of a specific PR body (PULL_REQ_ID will no longer be necessary and we can provide more context in the PR body).

Parent and Child are covered by #72 - All will keep the current strategy.

codecov · 2020-04-17T04:22:26Z

Codecov Report

Merging #142 into master will increase coverage by 0.46%.
The diff coverage is 62.89%.

@@             Coverage Diff              @@
##             master     #142      +/-   ##
============================================
+ Coverage     68.82%   69.28%   +0.46%     
- Complexity      140      176      +36     
============================================
  Files             7       13       +6     
  Lines           603      687      +84     
  Branches         93       99       +6     
============================================
+ Hits            415      476      +61     
- Misses          168      191      +23     
  Partials         20       20

Impacted Files	Coverage Δ	Complexity Δ
...ce/dockerfileimageupdate/subcommands/impl/All.java	`57.85% <33.33%> (-3.26%)`	`16.00 <1.00> (-1.00)`
...dockerfileimageupdate/subcommands/impl/Parent.java	`52.83% <34.48%> (-13.84%)`	`8.00 <5.00> (-12.00)`
...dockerfileimageupdate/storage/GitHubJsonStore.java	`37.50% <37.50%> (ø)`	`5.00 <5.00> (?)`
...sforce/dockerfileimageupdate/utils/GitHubUtil.java	`69.51% <54.54%> (-0.86%)`	`17.00 <1.00> (ø)`
...kerfileimageupdate/utils/DockerfileGitHubUtil.java	`77.77% <64.28%> (+8.59%)`	`39.00 <11.00> (ø)`
.../dockerfileimageupdate/utils/ResultsProcessor.java	`83.33% <83.33%> (ø)`	`3.00 <3.00> (?)`
...force/dockerfileimageupdate/repository/GitHub.java	`85.71% <85.71%> (ø)`	`7.00 <7.00> (?)`
...rce/dockerfileimageupdate/model/GitForkBranch.java	`88.88% <88.88%> (ø)`	`16.00 <16.00> (?)`
...leimageupdate/process/GitHubPullRequestSender.java	`88.88% <88.88%> (ø)`	`13.00 <13.00> (?)`
.../salesforce/dockerfileimageupdate/CommandLine.java	`53.26% <100.00%> (ø)`	`11.00 <1.00> (ø)`
... and 8 more

justinharringa · 2020-05-11T18:34:47Z

Note: There's likely a need for fork cleanup that should either be a part of this process or separate (to reduce specific time of this process). For example, the original parent repo could be deleted or moved which would put the fork in the bot org in a weird state (the deletion can make the fork in the bot org a non-fork).

It has proven wasteful and confusing to remove forks. Start marching towards using branches within a fork. Refactor out duplication to repository/GitHub class

This yields more results for other Dockerfile names

* Added slight logging improvement after PR created * Shift away from deprecated JsonParser methods

This is a steel thread approach. Needs a bit more refactoring and tests.

* Also improve unreachable code block in GitForkBranch.isSameBranchOrHasImageNamePrefis(branchName)

* JSON processing can move to specific GitHubJsonStore (theoretically we could support more stores and more than just GitHub) * DockerfileGitHubUtil could potentially start migrating to an interface to provide a dry run mode (but it has far too many responsibilities at the moment) * Pull generic algorithm out of Parent into GitHubPullRequestSender

afalko · 2020-05-17T22:00:49Z

...ile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/model/GitForkBranch.java

+        return false;
+    }
+
+    private String getBranchWithoutTag(String branchName) {


Are we documenting branch name conventions anywhere?

Yeah, I kinda put it up top but this should be nearer to the function. Adding.

afalko · 2020-05-17T22:02:29Z

...image-update/src/main/java/com/salesforce/dockerfileimageupdate/storage/GitHubJsonStore.java

+                }
+            }
+            String jsonOutput = getAndModifyJsonString(json, img, tag);
+            content.update(jsonOutput,


Doe this include retries?

Nah, it doesn't currently. I haven't observed many issues where retries set this in a bad spot. That is something that could be considered for sure. I would have a similar concern about determining when to retry to help reduce rate limit pressure.

afalko · 2020-05-17T22:04:43Z

...ge-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/DockerfileGitHubUtil.java

        }
+        // TODO: This may loop forever in the event of constant -1 pullRequestExitCodes...


Give it a configurable timeout?

Yeah, right now focusing on solidifying the move to stop forking and branch instead. This needs to be done at some point or another though.

afalko · 2020-05-17T22:05:52Z

...ge-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/DockerfileGitHubUtil.java

+            if (contentsWithImage.getTotalCount() > 0) {
+                break;
+            } else {
+                Thread.sleep(1000);


TimeUnit.SECONDS.sleep(1);

Change still not made?

Bah... There were a bunch... Corrected that crust

afalko · 2020-05-17T22:06:19Z

...erfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/GitHubUtil.java

-                    "provided has access to deleting repositories.");
+        if (!repo.queryPullRequests().state(GHIssueState.OPEN).list().iterator().hasNext()) {
+            try {
+                repo.delete();


Need to retry on failure a few times?

In which scenario would you expect that? Are you thinking of a network blip or momentary server unavailability? A potential issue with that would be to potentially put pressure on GitHub rate limits.

One thing that I'm considering to open up as an issue is to avoid forking at all if a repo doesn't need a change. We should be able to detect that off of the primary repo and then just skip forking it at the beginning.

Yeh, network blips or errors

* A failed user check shouldn't blow up the whole process

Also remove invalid test for Child subcommand

justinharringa · 2020-05-18T04:54:05Z

This PR already involves a fairly significant amount of change. Talked a bit with @afalko. Will let parent and child get these new changes and then all will kind of operate as it did before (though not a complete fork delete). Next PR will involve completely aligning all with parent.

afalko · 2020-05-18T05:22:44Z

...image-update/src/main/java/com/salesforce/dockerfileimageupdate/subcommands/impl/Parent.java

        }
    }
+    private Optional<GHPullRequest> commentAndCloseExistingPullRequestsForBranch(GHRepository parentRepo, GitForkBranch gitForkBranch) {
+        Optional<GHPullRequest> specifiedBranchPullRequest = dockerfileGitHubUtil.getPullRequestForImageBranch(parentRepo, gitForkBranch);
+        //            GHPullRequest pullRequest = specifiedBranchPullRequest.get();


Need to keep this commented?

afalko · 2020-05-18T05:23:28Z

...ge-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/DockerfileGitHubUtil.java

+            if (contentsWithImage.getTotalCount() > 0) {
+                break;
+            } else {
+                Thread.sleep(1000);


Change still not made?

justinharringa requested a review from afalko as a code owner April 17, 2020 04:20

justinharringa added the WIP label Apr 19, 2020

justinharringa force-pushed the dont-close-forks branch from 90c77dd to 778cfd1 Compare May 11, 2020 17:13

justinharringa added 6 commits May 16, 2020 12:57

Correct inputFromInstructionData DataProvider

cc2b159

Refactor out repository/GitHub, rename ensure fork

2b60133

It has proven wasteful and confusing to remove forks. Start marching towards using branches within a fork. Refactor out duplication to repository/GitHub class

Switch language:Dockerfile to filename:Dockerfile

9c22a1e

This yields more results for other Dockerfile names

Don't delete forks with PRs, GitHub API 1.111, etc

bc598d4

* Added slight logging improvement after PR created * Shift away from deprecated JsonParser methods

Move to newer ArgumentParsers.newFor

6d9290b

Convert parent command to use image-tag branch

214af2a

This is a steel thread approach. Needs a bit more refactoring and tests.

justinharringa force-pushed the dont-close-forks branch from 778cfd1 to 214af2a Compare May 16, 2020 19:57

justinharringa linked an issue May 16, 2020 that may be closed by this pull request

Fix idempotency and code duplication in All / Parent #71

Open

justinharringa added 6 commits May 16, 2020 13:35

Extract GitHubContentToProcess

9ef0e35

* Also improve unreachable code block in GitForkBranch.isSameBranchOrHasImageNamePrefis(branchName)

Extract ResultsProcessor class

ed0e8d5

Test ProcessResults has nothing to report

e120952

Extract reusable getGHContents from Parent

52f88e0

Move away from JsonParser deprecated methods

1e3856e

afalko reviewed May 17, 2020

View reviewed changes

justinharringa added 6 commits May 17, 2020 18:14

Improve GitForkBranch javadoc, fix magic string

0b5984a

Extract shouldNotForkRepo(repo), safe user check

fbf8fee

* A failed user check shouldn't blow up the whole process

Remove unnecessary comment

8456bd6

Slightly simpler shouldNotForkRepo(repo)

75f09e1

Extract ensureForkedAndAddToListForProcessing

348dbc5

Simplify ensureForkedAndAddToListForProcessing

4456b6f

justinharringa added 3 commits May 17, 2020 21:03

Extract getForkFromExistingRecordToProcess / test

c1eb278

Child subcommand will use new strategy

1198852

Gracefully handle repo not found when forking

4a5ee16

justinharringa removed a link to an issue May 18, 2020

Fix idempotency and code duplication in All / Parent #71

Open

All subcommand will continue to close PRs

1ff2203

Also remove invalid test for Child subcommand

This was referenced May 18, 2020

Evaluate adding retries for content updates #146

Open

Evaluate adding retries for fork deletion #147

Open

justinharringa changed the title ~~Don't close forks~~ Don't close forks, don't close PRs for child/parent commands May 18, 2020

justinharringa mentioned this pull request May 18, 2020

Set a timeout for when we might fail to create a pull request #148

Open

justinharringa removed the WIP label May 18, 2020

afalko requested changes May 18, 2020

View reviewed changes

justinharringa added 2 commits May 17, 2020 22:27

Convert 1000 magic number to TimeUnit

5a59922

Eliminate dead code from parent

691694a

afalko approved these changes May 18, 2020

View reviewed changes

afalko merged commit a542537 into master May 18, 2020

afalko deleted the dont-close-forks branch May 18, 2020 05:43

justinharringa added the improvement New features and improvements label Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't close forks, don't close PRs for child/parent commands #142

Don't close forks, don't close PRs for child/parent commands #142

justinharringa commented Apr 17, 2020 •

edited

Loading

codecov bot commented Apr 17, 2020 •

edited

Loading

justinharringa commented May 11, 2020

afalko May 17, 2020

justinharringa May 18, 2020

afalko May 17, 2020

justinharringa May 18, 2020

afalko May 17, 2020

justinharringa May 18, 2020

afalko May 17, 2020

afalko May 18, 2020

justinharringa May 18, 2020

afalko May 17, 2020

justinharringa May 18, 2020

afalko May 18, 2020

justinharringa commented May 18, 2020

afalko May 18, 2020

afalko May 18, 2020

		}
		// TODO: This may loop forever in the event of constant -1 pullRequestExitCodes...

Don't close forks, don't close PRs for child/parent commands #142

Don't close forks, don't close PRs for child/parent commands #142

Conversation

justinharringa commented Apr 17, 2020 • edited Loading

codecov bot commented Apr 17, 2020 • edited Loading

Codecov Report

justinharringa commented May 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinharringa commented May 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinharringa commented Apr 17, 2020 •

edited

Loading

codecov bot commented Apr 17, 2020 •

edited

Loading