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

AJ-1490, AJ-423: update workbench-libs and mockserver #1250

Merged
merged 9 commits into from
Dec 4, 2023

Conversation

davidangb
Copy link
Contributor

@davidangb davidangb commented Dec 2, 2023

This PR covers:

This PR intentionally does not cover other updates, such as updating workbench-libs in the automation/ subdirectory, or updating workbench-util (AJ-424), akka (AJ-421) or sentry libs (AJ-422). To keep scope down, those should be separate PRs.

Update Orch's three workbench-libs dependencies to latest versions.

This required a corresponding update to mockserver-netty. This update carries some syntax changes in test code. All of the code changes in this PR are purely for compatibility with the updates.


Have you read CONTRIBUTING.md lately? If not, do that first.

I, the developer opening this PR, do solemnly pinky swear that:

  • I've followed the instructions if I've made any changes to the API, especially if they're breaking changes
  • I've updated the RC_XXX release ticket with any manual steps required to release this change
  • I've updated the FISMA documentation if I've made any security-related changes, including auth, encryption, or auditing

In all cases:

  • Get two thumbsworth of review and PO signoff if necessary
  • Verify all tests go green
  • Squash and merge; you can delete your branch after this unless it's for a hotfix. In that case, don't delete it!
  • Test this change deployed correctly and works on dev environment after deployment

@@ -15,6 +15,7 @@ object Merging {
case x if x.endsWith("kotlin-stdlib.kotlin_module") => MergeStrategy.first
case x if x.contains("bouncycastle") => MergeStrategy.first
case x if x.endsWith("kotlin-stdlib-common.kotlin_module") => MergeStrategy.first
case x if x.endsWith("okio.kotlin_module") => MergeStrategy.first
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this fixes an issue in sbt assembly, which we run as part of deployment and tests, but developers typically do not run locally. sbt assembly compiles the fat jar, and hit an error trying to de-duplicate okio-jvm META-INF/okio.kotlin_module and okio META-INF/okio.kotlin_module. This fix takes the same approach as the merge strategy for kotlin-stdlib and otlin-stdlib-common just above in this file.

@@ -276,7 +277,7 @@ object MockWorkspaceServer {
request()
.withMethod("GET")
.withPath(s"${workspaceBasePath}/%s/%s/submissions/%s/workflows/%s"
.format(mockSpacedWorkspace.namespace, mockSpacedWorkspace.name, mockValidId, mockValidId)))
.format(mockSpacedWorkspace.namespace, UrlEscapers.urlPathSegmentEscaper().escape(mockSpacedWorkspace.name), mockValidId, mockValidId)))
Copy link
Contributor Author

@davidangb davidangb Dec 3, 2023

Choose a reason for hiding this comment

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

these are syntax/behavior differences in the updated mockserver library, as are almost all the other changes in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see you only had to do this URL escaping once in this PR...but it also looks pretty ugly -- if you had to do this 20 times, once for each test case, what would your solution look like?

I'm asking not because I want you to implement that solution (since this is just a one-off) but just curious what it'd look like in Scala :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would take one of two routes:

wrap the escaping in something terser, like StreamingPassthrough.escapePathSegment, so you end up with:

          .withPath(s"${workspaceBasePath}/%s/%s/submissions/%s/workflows/%s"
            .format(mockSpacedWorkspace.namespace, escapePathSegment(mockSpacedWorkspace.name), mockValidId, mockValidId)))

this isn't a big win, but it's nicer.

Alternately, I would make an implicit class that provides methods for strings; then, I could call my custom method directly on the thing I wanted to escape:

    implicit class StringEscaper(s: String) {
      def escape: String = UrlEscapers.urlPathSegmentEscaper().escape(s)
    }

…
          .withPath(s"${workspaceBasePath}/%s/%s/submissions/%s/workflows/%s"
            .format(mockSpacedWorkspace.namespace, mockSpacedWorkspace.name.escape, mockValidId, mockValidId)))

@@ -95,7 +95,7 @@ object Dependencies {
"com.github.pathikrit" %% "better-files" % "3.9.2",

"org.scalatest" %% "scalatest" % "3.2.17" % "test",
"org.mock-server" % "mockserver-netty" % "3.11" % "test", // TODO: upgrading higher causes failures, need to investigate
"org.mock-server" % "mockserver-netty" % "5.15.0" % "test",
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 workbench-libs updates hit a conflict with Orch's older version of mockserver, so I needed to update mockserver too.

Copy link

codecov bot commented Dec 3, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (995acf9) 69.36% compared to head (28c8da8) 69.92%.

❗ Current head 28c8da8 differs from pull request most recent head a8c6478. Consider uploading reports for the commit a8c6478 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1250      +/-   ##
===========================================
+ Coverage    69.36%   69.92%   +0.56%     
===========================================
  Files           97      101       +4     
  Lines         3401     3501     +100     
  Branches       381      391      +10     
===========================================
+ Hits          2359     2448      +89     
- Misses        1042     1053      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@davidangb davidangb changed the title AJ-1490: update workbench-libs and mockserver AJ-1490, AJ-423: update workbench-libs and mockserver Dec 3, 2023
@@ -6,7 +6,7 @@ object Dependencies {
val jacksonV = "2.13.4"
val jacksonHotfixV = "2.13.4.2" // for when only some of the Jackson libs have hotfix releases
val nettyV = "4.1.101.Final"
val workbenchLibsHash = "084d25b"
val workbenchLibsHash = "8ccaa6d"
Copy link
Contributor

Choose a reason for hiding this comment

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

Without reading anything else from this repo (eg: its readme), I am guessing this hash is just a github sha from the workbenchLibsRepo.

Might be worth an inline comment with a link to where that list of hashes is, for quicker lookup, assuming this is something that needs to be discovered & changed whenever we need to bump our dep here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a comment (and yes, it is in the workbench-libs readme)

@davidangb davidangb merged commit a011aa8 into develop Dec 4, 2023
9 checks passed
@davidangb davidangb deleted the da_AJ-1490_upgradeWorkbenchLibs branch December 4, 2023 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants