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.

Sign up for GitHub

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

allow test with pekko 1.0.x snapshot #333

Merged
merged 7 commits into from
Oct 29, 2023
Merged

allow test with pekko 1.0.x snapshot #333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2c64ef5
allow test with pekko 1.0.x snapshot
pjfanning Oct 18, 2023
43581ba
Update PekkoDependency.scala
pjfanning Oct 18, 2023
b4b6403
Update PekkoDependency.scala
pjfanning Oct 18, 2023
db4825c
rename config value to `1.0.x`
pjfanning Oct 29, 2023
ee44a1e
Update nightly.yml to include 1.0.x run
pjfanning Oct 29, 2023
28c3772
Merge branch 'main' into allow-test-with-pekko-10x-snapshot
pjfanning Oct 29, 2023
5a1a533
Update PekkoDependency.scala
pjfanning Oct 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/nightly.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ jobs:
matrix:
SCALA_VERSION: [2.12, 2.13, 3]
JDK: [8, 11, 17, 21]
PEKKO_VERSION: [default, main]
PEKKO_VERSION: ['default', 'main', '1.0.x']
steps:
- name: Checkout
uses: actions/checkout@v4
Expand Down
7 changes: 6 additions & 1 deletion project/PekkoDependency.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ object PekkoDependency {
case None =>
Option(System.getProperty("pekko.http.build.pekko.version")) match {
case Some("main") => mainSnapshot
case Some("1.0.x") => snapshot10x
case Some("default") | None => Artifact(defaultVersion)
case Some(other) => Artifact(other, true)
}
Expand All @@ -55,6 +56,7 @@ object PekkoDependency {
val default = pekkoDependency(defaultVersion = minimumExpectedPekkoVersion)
def docs = default

lazy val snapshot10x = Artifact(determineLatestSnapshot("1.0"), true)
lazy val mainSnapshot = Artifact(determineLatestSnapshot(), true)

val pekkoVersion: String = default match {
Expand Down Expand Up @@ -106,6 +108,8 @@ object PekkoDependency {
s"${Resolver.ApacheMavenSnapshotsRepo.root}org/apache/pekko/pekko-cluster-sharding-typed_2.13/")),
10.seconds).bodyAsString

// we use tagNumber set as Integer.MAX_VALUE when there is no tagNumber
// this ensures that RC and Milistone versions are treated as older than non-RC/non-milestone versions
val allVersions =
snapshotVersionR.findAllMatchIn(body)
.map {
Expand All @@ -114,11 +118,12 @@ object PekkoDependency {
ep.toInt,
maj.toInt,
min.toInt,
Option(tagNumber).map(_.toInt),
Option(tagNumber).map(_.toInt).getOrElse(Integer.MAX_VALUE),
Copy link
Contributor

@mdedetrich mdedetrich Oct 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why was this change needed? Is that part of

adjusts the tagNumber logic to treat build numbers without an RC or M tag as newer than ones that have then?

Don't see how using Integer.MAX_VALUE is correct for what its doing (i.e. testing against latest found snapshot)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes - this is related to adjusts the tagNumber logic to treat build numbers without an RC or M tag as newer than ones that have them

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, but how is Integer.MAX_VALUE going to provide a correct value here, or am I missing something? Is it just some sentinel value that will get ignored later (if so add a comment because logic/reasoning behind it is unclear).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nvm, its for ordering and not the actual value. @pjfanning Can you just add a comment explaining that its there to fix ordering and then ill approve PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The values are used only in sorting.

  • -RC1- will eval as 1
  • no -RC or -M will now eval as MAX-INT
  • MAX-INT > 1 so sorting will treat non-RC/non-milestone releases as newer

Old logic:

  • -RC1- will eval as Some(1)
  • no -RC or -M will eval as None
  • None < Some(1) so sorting will treat non-RC/non-milestone releases as older

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood, just add a comment in the code quickly explaining this because on first glance its misleading.

offset.toInt) -> full
}
.filter(_._2.startsWith(prefix))
.toVector.sortBy(_._1)

allVersions.last._2
}
}
Loading