Proposal: Improve branch selection #2281

alejandrohdezma · 2021-10-13T13:17:09Z

What has been done in this PR?

This PR improves the branch selection feature added in #2183 by allowing to be used not only for users of the scala-steward-action or users that run their own instance of Steward, but to anyone using the repos.md feature.

How?

Instead of receiving the default branch to use from the CLI, I've updated the repos.md parser to also allow the following syntax:

- organization/repository:branch

The Repo class has been updated with a new field branch: Option[Branch]. If that field is present, that branch will be used as the default branch instead of actual default branch.

What does this change mean?

This change means that repositories like cats or http4s (or any repository using multibranch) can maintain several branches updated with Scala Steward by updating the repos.md line to:

- http4s/http4s
+ http4s/http4s:main
+ http4s/http4s:0.23.x
+ http4s/http4s:0.22.x
+ http4s/http4s:0.21.x

Isn't this a breaking change?

Removing the CLI param doesn't provoke a breaking change, since a new version hasn't been released since #2183 was merged 😸.

Want to see it in action?

Here you can see this new feature in action. Steward was run with the following parameters:

--do-not-fork --workspace workspace --repos-file repos.md --git-author-email [email protected] --vcs-login scala-steward --git-ask-pass askpass.sh

with repos.md being:

- alejandrohdezma/scala-steward-new-feature-test
- alejandrohdezma/scala-steward-new-feature-test:0.21.x
- alejandrohdezma/scala-steward-new-feature-test:0.22.x

As you can see here Scala Steward created 3 PRs, each pointing to a different branch, and each one of them using the .scala-steward.conf present in that branch.

Signed-off-by: Alejandro Hernández <[email protected]>

codecov · 2021-10-13T13:27:19Z

Codecov Report

Merging #2281 (2cc0c9b) into master (4939946) will decrease coverage by 0.00%.
The diff coverage is 70.37%.

@@            Coverage Diff             @@
##           master    #2281      +/-   ##
==========================================
- Coverage   77.83%   77.82%   -0.01%     
==========================================
  Files         135      135              
  Lines        2287     2291       +4     
  Branches       51       50       -1     
==========================================
+ Hits         1780     1783       +3     
- Misses        507      508       +1

Impacted Files	Coverage Δ
.../scala/org/scalasteward/core/application/Cli.scala	`96.15% <ø> (ø)`
...ala/org/scalasteward/core/nurture/NurtureAlg.scala	`0.00% <0.00%> (ø)`
...org/scalasteward/core/repocache/RepoCacheAlg.scala	`0.00% <0.00%> (ø)`
...teward/core/vcs/bitbucket/RepositoryResponse.scala	`87.50% <33.33%> (-3.81%)`	⬇️
...org/scalasteward/core/application/StewardAlg.scala	`44.44% <50.00%> (+0.32%)`	⬆️
...ala/org/scalasteward/core/application/Config.scala	`97.56% <100.00%> (-0.06%)`	⬇️
...ain/scala/org/scalasteward/core/edit/EditAlg.scala	`60.60% <100.00%> (+1.23%)`	⬆️
...main/scala/org/scalasteward/core/git/package.scala	`100.00% <100.00%> (ø)`
...in/scala/org/scalasteward/core/vcs/VCSApiAlg.scala	`100.00% <100.00%> (ø)`
...n/scala/org/scalasteward/core/vcs/VCSRepoAlg.scala	`76.47% <100.00%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4939946...2cc0c9b. Read the comment docs.

Signed-off-by: Alejandro Hernández <[email protected]>

…lt one Signed-off-by: Alejandro Hernández <[email protected]>

fthomas · 2021-10-13T18:58:18Z

I really would like to have this feature in Scala Steward but more changes are needed to properly support multiple branches. (Some of them were in #1469.) For example, for each branch we need to have different RepoCaches that are persisted to disk. I think in this PR, the RepoCaches for each branch is persisted to the same file and thus overwriting the cache of a prior branch.

alejandrohdezma · 2021-10-14T04:02:31Z

I can try to include the changes in the linked PR

Signed-off-by: Alejandro Hernández <[email protected]>

alejandrohdezma · 2021-10-16T08:34:46Z

Hey @fthomas I've already included all the missing changes in #1469. I believe the cache issue is resolved, given that the cache is created using the Repo as key, and the Repo contains the custom branch (if any is present), but if I'm misinterpreting something, let me know.

The only missing thing would be to try this using fork (I've already tried it using the doNotFork). Do you have any kind of playground for testing this kind of changes? If not, I can set something up quickly.

Once we are happy with the solution, I can update README and documentation, and create a PR for the GitHub Action.

Signed-off-by: Alejandro Hernández <[email protected]>

alejandrohdezma · 2021-10-16T08:47:33Z

I've checked with this test repo (using --do-not-fork) and the cache is created succesfully. It creates 3 repo_cache.json files:

workspace/store/repo_cache/v1/github/alejandrohdezma/scala-steward-new-feature-test/repo_cache.json
workspace/store/repo_cache/v1/github/alejandrohdezma/scala-steward-new-feature-test/0.21.x/repo_cache.json
workspace/store/repo_cache/v1/github/alejandrohdezma/scala-steward-new-feature-test/0.22.x/repo_cache.json

alejandrohdezma · 2021-10-16T08:50:53Z

Also, it seems the KeyDecoder[Repo] is unused. Should I remove it? Or is it there for some reason?

fthomas · 2021-10-16T12:31:48Z

Also, it seems the KeyDecoder[Repo] is unused. Should I remove it? Or is it there for some reason?

It was added in this ancient commit and was required for the Decoder[Store] instance there. It was then later required for the JsonKeyValueStore which lost that requirement in #1220. It looks indeed unused now and can be removed.

fthomas · 2021-10-16T12:43:35Z

The only missing thing would be to try this using fork (I've already tried it using the doNotFork). Do you have any kind of playground for testing this kind of changes? If not, I can set something up quickly.

I could run it locally on https://github.com/scala-steward-org/test-repo-2 if you like. I created a new test-branch in that repo. If you can add some different dependencies to master and test-branch and update the repos.md in this PR, it is easy for me to try it out.

fthomas · 2021-10-16T14:26:32Z

I just tried it locally:

[info] 2021-10-16 16:15:11,582 INFO  ──────────── Steward scala-steward-org/test-repo-2/test-branch ────────────
[info] 2021-10-16 16:15:11,583 INFO  Check cache of scala-steward-org/test-repo-2/test-branch
[info] 2021-10-16 16:15:12,184 INFO  Clone scala-steward/test-repo-2
[info] 2021-10-16 16:15:12,876 ERROR Steward scala-steward-org/test-repo-2/test-branch failed
[info] java.io.IOException: 'git checkout test-branch' exited with code 1

syncFork already checks out a branch. Maybe parent.default_branch just needs to be replaced with repo.branch.getOrElse(parent.default_branch)?

This is unused since #1220. See #2281 (comment).

This adds `Repo.toPath` which has currently the same implementation as `Repo.show`. `show` is used as representation in log messages while `toPath` is used as path components. This allows us in #2281 to use `owner/repo:branch` for `show` and `owner/repo/branch` for `toPath`.

modules/core/src/main/scala/org/scalasteward/core/vcs/package.scala

…ch-selection-to-repos-file Signed-off-by: Alejandro Hernández <[email protected]> # Conflicts: # modules/core/src/main/scala/org/scalasteward/core/vcs/data/Repo.scala # modules/core/src/main/scala/org/scalasteward/core/vcs/package.scala # modules/core/src/test/scala/org/scalasteward/core/vcs/data/RepoTest.scala

modules/core/src/main/scala/org/scalasteward/core/vcs/data/Repo.scala

fthomas · 2021-10-17T07:35:01Z

Maybe parent.default_branch just needs to be replaced with repo.branch.getOrElse(parent.default_branch)?

I'll revert this since parent.default_branch is already the same as repo.branch. I didn't realized that createForkOrGetRepoWithBranch already modified the parent's default_branch.

modules/core/src/test/scala/org/scalasteward/core/git/gitTest.scala

modules/core/src/main/scala/org/scalasteward/core/git/package.scala

Signed-off-by: Alejandro Hernández <[email protected]>

alejandrohdezma · 2021-10-17T11:00:54Z

I've added this PR in the scala-steward-action to support this new feature.

alejandrohdezma · 2021-10-17T11:54:36Z

I noticed this feature didn't take into account users running Scala Steward for a GitHub App. For those users the only option would be to have something in the repo (some kind of file) that lists the branches to update and request that file via API.

The only problem is that the REST API has a rate-limit of 5000 calls per-hour, so I investigated it using the GraphQL API (that allows to query several repos at once) and this is the output:

So to sum up, we could change the way the branch is added, so it is added to the .scala-steward.conf file (instead of adding :branch) to the repos list. And read and parse that file at startup for all repos in the list, reading a new option branches from that file.

What do you think @fthomas?

alejandrohdezma · 2021-10-17T12:04:44Z

For example this is the output for the first 100 repositories listed in the GitHub repos.md file:

{
  "data": {
    "r1": {
      "object": null
    },
    "r2": {
      "object": null
    },
    "r3": {
      "object": null
    },
    "r4": {
      "object": null
    },
    "r5": {
      "object": null
    },
    "r6": {
      "object": null
    },
    "r7": {
      "object": {
        "text": "updates.includeScala = true\n"
      }
    },
    "r8": {
      "object": null
    },
    "r9": {
      "object": null
    },
    "r10": {
      "object": {
        "text": "updates.ignore = [\n  { groupId = \"com.typesafe\", artifactId = \"ssl-config-core\" },\n\n  { groupId = \"com.typesafe.akka\", artifactId = \"akka-actor\" },\n  { groupId = \"com.typesafe.akka\", artifactId = \"akka-actor-typed\" },\n  { groupId = \"com.typesafe.akka\", artifactId = \"akka-discovery\" },\n  { groupId = \"com.typesafe.akka\", artifactId = \"akka-slf4j\" },\n  { groupId = \"com.typesafe.akka\", artifactId = \"akka-stream\" },\n  { groupId = \"com.typesafe.akka\", artifactId = \"akka-stream-testkit\" },\n  { groupId = \"com.typesafe.akka\", artifactId = \"akka-testkit\" },\n  { groupId = \"com.typesafe.akka\", artifactId = \"akka-multi-node-testkit\" },\n\n  { groupId = \"com.typesafe.akka\", artifactId = \"akka-http\" },\n  { groupId = \"com.typesafe.akka\", artifactId = \"akka-http-core\" },\n\n  { groupId = \"org.scalatest\", artifactId = \"scalatest\" },\n]\n\nupdates.pin = [\n  { groupId = \"org.scalatest\", artifactId = \"scalatest\", version = \"3.1.\" },\n]\n"
      }
    },
    "r11": {
      "object": null
    },
    "r12": {
      "object": null
    },
    "r13": {
      "object": null
    },
    "r14": {
      "object": null
    },
    "r15": {
      "object": null
    },
    "r16": {
      "object": null
    },
    "r17": {
      "object": null
    },
    "r18": {
      "object": {
        "text": "updates.ignore = [ { groupId = \"org.apache.spark\", artifactId = \"spark-sql\" } ]\n"
      }
    },
    "r19": {
      "object": null
    },
    "r20": {
      "object": null
    },
    "r21": {
      "object": null
    },
    "r22": {
      "object": null
    },
    "r23": {
      "object": {
        "text": "updates.ignore = [ { groupId = \"org.apache.spark\", artifactId = \"spark-sql\" } ]\n"
      }
    },
    "r24": {
      "object": null
    },
    "r25": {
      "object": null
    },
    "r26": {
      "object": null
    },
    "r27": {
      "object": null
    },
    "r28": {
      "object": null
    },
    "r29": {
      "object": null
    },
    "r30": {
      "object": null
    },
    "r31": {
      "object": null
    },
    "r32": {
      "object": null
    },
    "r33": {
      "object": null
    },
    "r34": {
      "object": null
    },
    "r35": {
      "object": null
    },
    "r36": {
      "object": null
    },
    "r37": {
      "object": null
    },
    "r38": {
      "object": null
    },
    "r39": {
      "object": null
    },
    "r40": {
      "object": null
    },
    "r41": {
      "object": null
    },
    "r42": {
      "object": null
    },
    "r43": {
      "object": null
    },
    "r44": {
      "object": {
        "text": "updatePullRequests = \"always\"\ncommits.message = \":arrow_up: update ${artifactName} from ${currentVersion} to ${nextVersion}\"\n"
      }
    },
    "r45": {
      "object": null
    },
    "r46": {
      "object": null
    },
    "r47": {
      "object": null
    },
    "r48": {
      "object": null
    },
    "r49": {
      "object": null
    },
    "r50": {
      "object": null
    },
    "r51": {
      "object": null
    },
    "r52": {
      "object": null
    },
    "r53": {
      "object": null
    },
    "r54": {
      "object": null
    },
    "r55": {
      "object": null
    },
    "r56": {
      "object": null
    },
    "r57": {
      "object": null
    },
    "r58": {
      "object": null
    },
    "r59": {
      "object": null
    },
    "r60": {
      "object": null
    },
    "r61": {
      "object": null
    },
    "r62": {
      "object": null
    },
    "r63": {
      "object": null
    },
    "r64": {
      "object": null
    },
    "r65": {
      "object": null
    },
    "r66": {
      "object": null
    },
    "r67": {
      "object": null
    },
    "r68": {
      "object": null
    },
    "r69": {
      "object": null
    },
    "r70": {
      "object": null
    },
    "r71": {
      "object": null
    },
    "r72": {
      "object": null
    },
    "r73": {
      "object": null
    },
    "r74": {
      "object": null
    },
    "r75": {
      "object": null
    },
    "r76": {
      "object": null
    },
    "r77": {
      "object": null
    },
    "r78": {
      "object": null
    },
    "r79": {
      "object": null
    },
    "r80": {
      "object": null
    },
    "r81": {
      "object": null
    },
    "r82": {
      "object": null
    },
    "r83": {
      "object": null
    },
    "r84": {
      "object": null
    },
    "r85": {
      "object": null
    },
    "r86": {
      "object": null
    },
    "r87": {
      "object": null
    },
    "r88": {
      "object": null
    },
    "r89": {
      "object": null
    },
    "r90": {
      "object": null
    },
    "r91": {
      "object": null
    },
    "r92": {
      "object": {
        "text": "updates.pin  = [ { groupId = \"org.typelevel\", artifactId=\"cats-effect\", version = \"2.\" } ]\n"
      }
    },
    "r93": {
      "object": null
    },
    "r94": {
      "object": null
    },
    "r95": {
      "object": null
    },
    "r96": {
      "object": null
    },
    "r97": {
      "object": null
    },
    "r98": {
      "object": null
    },
    "r99": {
      "object": null
    },
    "r100": {
      "object": null
    }
  }
}

We would just need to find something similar for GitLab/BitBucket users. Although given the amount of usage those have for the public Scala Steward, we could just go with doing one call per repository to retrieve the file.

fthomas · 2021-10-17T15:16:02Z

I noticed this feature didn't take into account users running Scala Steward for a GitHub App.

Oh nooo. 😱 That's the curse of having so many features. Can't we just not support branch selection if it used as GitHub App?

alejandrohdezma · 2021-10-17T18:34:52Z

I noticed this feature didn't take into account users running Scala Steward for a GitHub App.

Oh nooo. 😱 That's the curse of having so many features. Can't we just not support branch selection if it used as GitHub App?

I suppose. Whatever you prefer, I don't mind trying to implement it. The possible benefit if we can make this work would be that nobody would need to send a PR for updating the repos.md for supporting multi-branch, just adding a new field to the .scala-steward.conf.

But if you are happy with not supporting multi-branch for GitHub Apps and continue with the current implementation it's fine by me 😸

fthomas · 2021-10-17T18:58:42Z

The possible benefit if we can make this work would be that nobody would need to send a PR for updating the repos.md for supporting multi-branch, just adding a new field to the .scala-steward.conf.

What I like about the current approach is that no branch is really special. It is possible to get updates for a branch that is not the default branch without configuring anything in the default branch. If branches are configured via .scala-steward.conf, I guess the default branch becomes special because only it can be used to configure the list of branches that should receive updates.

But if you are happy with not supporting multi-branch for GitHub Apps and continue with the current implementation it's fine by me

Great, then let's continue with the current implementation!

This reverts commit 00e942f.

fthomas

Outstanding work @alejandrohdezma!

fthomas · 2021-10-18T03:08:08Z

I'd be happy to merge this PR now. @alejandrohdezma is there anything left you want to do before I hit the merge button?

alejandrohdezma · 2021-10-18T04:39:50Z

Nothing that I can think of :)

fthomas · 2021-10-19T06:28:47Z

This feature works great! Here are two random PRs in the same repo targeting different branches:

alejandrohdezma · 2021-10-19T09:13:46Z

How is the feature working, since a new version hasn't been published yet?

fthomas · 2021-10-19T09:26:36Z

My public instance tracks master and does not wait for releases. So every new feature is available instantly.

Should we publish a new version for the GH Action soon?

alejandrohdezma · 2021-10-19T09:29:28Z

Oh, that's nice. Yeah, that would be super :)

I'm adding a PR for supporting the new --default-repo-conf from the GH Action too, so it would be nice to release both features at the same time :)

fthomas · 2021-10-19T10:30:11Z

0.12.0 is now available.

alejandrohdezma added 5 commits October 13, 2021 15:21

Add optional Branch param to Repo class

6e4d716

Signed-off-by: Alejandro Hernández <[email protected]>

Populate branch for repos added as - org/repo:branch

d7f5338

Signed-off-by: Alejandro Hernández <[email protected]>

Use branch from Repo class instead of configuration

0449cbc

Signed-off-by: Alejandro Hernández <[email protected]>

Remove --default-branch CLI param

ab00c61

Signed-off-by: Alejandro Hernández <[email protected]>

Run sbt fmt

b0a8e0a

Signed-off-by: Alejandro Hernández <[email protected]>

alejandrohdezma added 2 commits October 13, 2021 15:49

Update Repo#show to include branch information if present

ee9ab63

Signed-off-by: Alejandro Hernández <[email protected]>

Ensure PR title includes the branch if it is different than the defau…

88015c9

…lt one Signed-off-by: Alejandro Hernández <[email protected]>

alejandrohdezma added 2 commits October 16, 2021 10:27

Propagate branch to missing places

2f21827

Signed-off-by: Alejandro Hernández <[email protected]>

Merge branch 'master' into move-default-branch-selection-to-repos-file

02266c3

alejandrohdezma added 2 commits October 16, 2021 10:35

Run sbt fmt

0ac47d4

Signed-off-by: Alejandro Hernández <[email protected]>

Remove unused import

c4bf4e5

Signed-off-by: Alejandro Hernández <[email protected]>

fthomas added a commit that referenced this pull request Oct 16, 2021

Remove KeyDecoder[Repo] instance

33135b9

This is unused since #1220. See #2281 (comment).

fthomas mentioned this pull request Oct 16, 2021

Remove KeyDecoder[Repo] instance #2291

Merged

fthomas mentioned this pull request Oct 16, 2021

Add Repo.toPath #2292

Merged

fthomas reviewed Oct 16, 2021

View reviewed changes

modules/core/src/main/scala/org/scalasteward/core/vcs/package.scala Outdated Show resolved Hide resolved

fthomas added the enhancement New feature or request label Oct 16, 2021

fthomas linked an issue Oct 16, 2021 that may be closed by this pull request

support branch selection #20

Closed

fthomas added this to the 0.12.0 milestone Oct 16, 2021

fthomas reviewed Oct 16, 2021

View reviewed changes

modules/core/src/main/scala/org/scalasteward/core/vcs/data/Repo.scala Outdated Show resolved Hide resolved

fthomas added 3 commits October 17, 2021 09:36

parent.default_branch is already the same as repo.branch

462e97f

Fix format of the example repos.md file

f1150d9

Minimize diff

0c82713

fthomas reviewed Oct 17, 2021

View reviewed changes

modules/core/src/test/scala/org/scalasteward/core/git/gitTest.scala Show resolved Hide resolved

fthomas reviewed Oct 17, 2021

View reviewed changes

modules/core/src/main/scala/org/scalasteward/core/git/package.scala Show resolved Hide resolved

fthomas and others added 2 commits October 17, 2021 10:44

Shorter show and toPath implementations

dfe8b4c

Add .vscode folder to .gitignore

104307a

Signed-off-by: Alejandro Hernández <[email protected]>

alejandrohdezma mentioned this pull request Oct 17, 2021

Add branches input to support updating multiple branches scala-steward-org/scala-steward-action#290

Merged

Propagate UpdateDate#baseBranch to commitMsgFor

00e942f

Signed-off-by: Alejandro Hernández <[email protected]>

fthomas added 2 commits October 17, 2021 22:18

Revert "Propagate UpdateDate#baseBranch to commitMsgFor"

7386dbc

This reverts commit 00e942f.

Add val for commit message

c6d7605

fthomas approved these changes Oct 17, 2021

View reviewed changes

Fix Repo.{show, toPath}

2cc0c9b

fthomas merged commit e96e0ce into scala-steward-org:master Oct 18, 2021

fthomas mentioned this pull request Oct 18, 2021

support branch selection #20

Closed

alejandrohdezma deleted the move-default-branch-selection-to-repos-file branch October 18, 2021 06:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal: Improve branch selection #2281

Proposal: Improve branch selection #2281

alejandrohdezma commented Oct 13, 2021 •

edited

Loading

codecov bot commented Oct 13, 2021 •

edited

Loading

fthomas commented Oct 13, 2021

alejandrohdezma commented Oct 14, 2021

alejandrohdezma commented Oct 16, 2021

alejandrohdezma commented Oct 16, 2021

alejandrohdezma commented Oct 16, 2021

fthomas commented Oct 16, 2021

fthomas commented Oct 16, 2021

fthomas commented Oct 16, 2021 •

edited

Loading

fthomas commented Oct 17, 2021

alejandrohdezma commented Oct 17, 2021

alejandrohdezma commented Oct 17, 2021

alejandrohdezma commented Oct 17, 2021

fthomas commented Oct 17, 2021

alejandrohdezma commented Oct 17, 2021

fthomas commented Oct 17, 2021

fthomas left a comment

fthomas commented Oct 18, 2021

alejandrohdezma commented Oct 18, 2021

fthomas commented Oct 19, 2021

alejandrohdezma commented Oct 19, 2021

fthomas commented Oct 19, 2021

alejandrohdezma commented Oct 19, 2021

fthomas commented Oct 19, 2021

Proposal: Improve branch selection #2281

Proposal: Improve branch selection #2281

Conversation

alejandrohdezma commented Oct 13, 2021 • edited Loading

What has been done in this PR?

How?

What does this change mean?

Isn't this a breaking change?

Want to see it in action?

codecov bot commented Oct 13, 2021 • edited Loading

Codecov Report

fthomas commented Oct 13, 2021

alejandrohdezma commented Oct 14, 2021

alejandrohdezma commented Oct 16, 2021

alejandrohdezma commented Oct 16, 2021

alejandrohdezma commented Oct 16, 2021

fthomas commented Oct 16, 2021

fthomas commented Oct 16, 2021

fthomas commented Oct 16, 2021 • edited Loading

fthomas commented Oct 17, 2021

alejandrohdezma commented Oct 17, 2021

alejandrohdezma commented Oct 17, 2021

alejandrohdezma commented Oct 17, 2021

fthomas commented Oct 17, 2021

alejandrohdezma commented Oct 17, 2021

fthomas commented Oct 17, 2021

fthomas left a comment

Choose a reason for hiding this comment

fthomas commented Oct 18, 2021

alejandrohdezma commented Oct 18, 2021

fthomas commented Oct 19, 2021

alejandrohdezma commented Oct 19, 2021

fthomas commented Oct 19, 2021

alejandrohdezma commented Oct 19, 2021

fthomas commented Oct 19, 2021

alejandrohdezma commented Oct 13, 2021 •

edited

Loading

codecov bot commented Oct 13, 2021 •

edited

Loading

fthomas commented Oct 16, 2021 •

edited

Loading