Migrates [Nexus] service to new service model #2520

calebcartwright · 2018-12-13T01:18:27Z

Ports the Nexus service to the new service model. Some related/relevant conversation in #2347 (and closes #2347). Also adds support for authentication which resolves #1699

I also added some thoughts/comments inline

shields-ci · 2018-12-13T01:19:30Z

	Messages
📖	✨ Thanks for your contribution to Shields, @calebcartwright!

Generated by 🚫 dangerJS against c9f38b2

services/nexus/nexus.service.js

services/nexus/nexus.tester.js

services/nexus/nexus.service.js

calebcartwright · 2018-12-18T21:58:44Z

~~I'm currently making~~ I added some updates to this based on some discussions/work from other threads (tests for color validation, tests for auth validation, etc.)

paulmelnikow

Thanks so much for this!

paulmelnikow · 2018-12-18T23:45:39Z

services/nexus/nexus.service.js

-const LegacyService = require('../legacy-service')
-const { makeBadgeData: getBadgeData } = require('../../lib/badge-data')
+const BaseJsonService = require('../base-json')
+const Joi = require('joi')
 const { isSnapshotVersion: isNexusSnapshotVersion } = require('./nexus-version')


Not necessary for this PR, though for future reference I've been trying to remove these renames as services are rewritten. (The renames are legacy too.)

services/nexus/nexus.service.js

paulmelnikow · 2018-12-18T23:51:47Z

services/nexus/nexus.service.js

+    })
+    const json = await this._requestJson(requestParams)
+    if (json.data.length === 0) {
+      return this.constructor.render({ version: 'no-artifact' })


This is not really a version, it's an error. Maybe it would be better to handle this by throwing either NotFound or InvalidResponse with a custom prettyMessage.

Agreed. This is the part I was referencing in my comment above on the Joi schema (not adding a min(1) ) to maintain consistency with the existing implementation.

A NotFound seems like the most logical error to me. Sounds like I can throw that with a custom message of no-artifact to maintain the same behavior (which currently is nexus | no-artifact)?

It seems like a strangely formatted message; feel free to change it.

paulmelnikow · 2018-12-18T23:52:52Z

services/nexus/nexus.service.js

+      message = versionText(version)
+      color = versionColor(version)
+    } else {
+      message = 'undefined'


This seems like an invalid response. Could it be handled with Joi instead? ^^

I honestly don't know on this one (relying on the LegacyService implementation) 😄

Based on a quick mental walkthrough of the Legacy implementation, I think this could happen under two circumstances:

snapshot badge is requested, but the specified group/artifact has no snapshot versions.

repository badge is requested, response has no value for baseVersion field (which I am assuming can/does happen in real life based on this line from the legacy service), nor does the response have a value for the version field.

The first one feels a lot like the above scenario where throwing NotFound could be used. It could be covered with a schema (although it would get a little complex), but I think a pretty message conveying no snapshot versions found might be more helpful for the user than invalid. Thoughts?

The second one would be much more manageable with Joi than the first IMO, but I'm not sure what Nexus guarantees in the response. For example is baseVersion optional while version is guaranteed? Or is it that one value is guaranteed, but it could be either version or baseVersion, or neither?

I'm familiar with what Nexus is, but haven't used it, so I'm gonna have to think about this when I'm a little fresher. If anyone else has another opinion meanwhile, please feel free to respond!

I added some links below to the Neuxs API docs, but they didn't offer much assistance IMO. I updated the service impl. to throw InvalidResponse errors in each of those 2 circumstances while leaving the Joi schema a bit more flexible

paulmelnikow · 2018-12-18T23:56:34Z

services/nexus/nexus.service.js

+    if (repo === 'r') {
+      version = json.data[0].latestRelease
+    } else if (repo === 's') {
+      json.data.every(artifact => {


This is fine, though for future reference you should know about another pattern we use, which is transform. If you delegate this part of the responsibility to a transform method, you can use a for loop with a return statement which is easier to read.

This was another copy/paste from the original implementation. I'm not familiar with transform but happy to optimize!

Is this an example of the transform you are referring to?

Yea, that's a good example. We should probably link that from the service-rewriting docs if there isn't already an example of a transform() function.

services/nexus/nexus.service.js

paulmelnikow · 2018-12-19T00:01:42Z

services/nexus/nexus.service.js

+      options.qs.v = 'LATEST'
+    }
+
+    if (queryOpt) {


It's not obvious from reading this what it's supposed to do. For future reference, stuff like this probably should be its own static method (or function) with a unit test.

Good idea. The original implementation was plugging the query params directly into the url, but that was a lot more readable.

I'll extract this out into its own function with a comment. 🤦‍♂️ on the test. Not sure how I forgot that one!

paulmelnikow · 2018-12-19T00:03:33Z

services/nexus/nexus.tester.js

    })
  )

-t.create('search release version of an inexistent artifact')
+t.create('live: search release version of an inexistent artifact')


This comes up a lot. Maybe we should add something to the test runner which includes [mock] next to all the ones which use mocks (or vice versa).

Makes sense. I saw the live: prefix convention somewhere the other day and started using it too without thinking about it.

services/nexus/nexus.tester.js

calebcartwright · 2018-12-19T03:08:08Z

Thanks for the feedback! I think I fixed each of the line items but let me know if I missed any

paulmelnikow · 2018-12-19T03:52:21Z

services/nexus/nexus.service.js

+    })
+  }
+
+  getRequestParams({ repo, scheme, host, groupId, artifactId, queryOpt }) {


This probably would be a bit more direct if it invoked _requestJson and was renamed to fetch. "Fetch" is another pattern we use a lot.

paulmelnikow · 2018-12-19T04:00:21Z

services/nexus/nexus.service.js

+    }
+
+    if (!version) {
+      throw new InvalidResponse({ prettyMessage: 'invalid artifact version' })


It's not necessary to change this, though if you move the error-checking bit back to handle(), you can use return statements above.

paulmelnikow · 2018-12-19T04:01:30Z

services/nexus/nexus.tester.js

+        r: 'fs-public-snapshots',
+        v: 'LATEST',
+        c: 'agent-apple-osx',
+        p: 'tar.gz',


paulmelnikow

All looks good!

Only thing that needs more discussion is this item: #2520 (comment)

calebcartwright · 2018-12-19T17:54:23Z

I can't find anything definitive in the Nexus API docs (for 2.x) on the response formats either:

On the Resolve API:
https://repository.sonatype.org/nexus-restlet1x-plugin/default/docs/path__artifact_maven_resolve.html
https://repository.sonatype.org/nexus-restlet1x-plugin/default/docs/el_ns0_artifact-resolution.html

On the Search API:
https://repository.sonatype.org/nexus-indexer-lucene-plugin/default/docs/path__lucene_search.html
https://repository.sonatype.org/nexus-indexer-lucene-plugin/default/docs/el_ns0_searchNGResponse.html

shields-deployment · 2018-12-19T20:45:26Z

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

paulmelnikow · 2018-12-19T20:45:32Z

Thanks! This was a beast!

calebcartwright added 3 commits December 12, 2018 19:05

feat: ported Nexus to new service model

eb8c0f6

feat: updated nexus service schema validation

09144a9

chore: minor tweak to nexus examples

c394c8c

calebcartwright commented Dec 13, 2018

View reviewed changes

services/nexus/nexus.service.js Show resolved Hide resolved

calebcartwright commented Dec 13, 2018

View reviewed changes

services/nexus/nexus.service.js Show resolved Hide resolved

calebcartwright commented Dec 13, 2018

View reviewed changes

services/nexus/nexus.service.js Show resolved Hide resolved

calebcartwright commented Dec 13, 2018

View reviewed changes

services/nexus/nexus.service.js Outdated Show resolved Hide resolved

calebcartwright commented Dec 13, 2018

View reviewed changes

services/nexus/nexus.tester.js Show resolved Hide resolved

feat: add authentication for nexus service

eae8940

calebcartwright commented Dec 13, 2018

View reviewed changes

services/nexus/nexus.service.js Outdated Show resolved Hide resolved

chore: fixed prettier issue on nexus service

ad736cc

paulmelnikow added the service-badge Accepted and actionable changes, features, and bugs label Dec 13, 2018

calebcartwright and others added 2 commits December 13, 2018 19:10

Merge branch 'master' into master

1b82548

chore: minor update for nexus badge examples

7e96daa

calebcartwright mentioned this pull request Dec 16, 2018

The Great Service Rewrite 🏆 #1358

Closed

calebcartwright added 3 commits December 17, 2018 15:05

Merge branch 'master' into master

0146680

Merge branch 'master' into master

2292c8c

Merge branch 'master' into master

9a10be1

calebcartwright and others added 4 commits December 18, 2018 16:32

Merge branch 'master' into master

6833790

feat: updated nexus service and tests

fbb7ea9

Merge branch 'master' of https://github.com/calebcartwright/shields

1c8fb0c

chore: remove duplicative keywords from nexus service

1a31178

paulmelnikow reviewed Dec 19, 2018

View reviewed changes

feat: updated nexus service

5aed5e3

paulmelnikow mentioned this pull request Dec 19, 2018

Prefix the titles of mocked tests in the runner #2555

Closed

paulmelnikow reviewed Dec 19, 2018

View reviewed changes

services/nexus/nexus.tester.js

r: 'fs-public-snapshots',

v: 'LATEST',

c: 'agent-apple-osx',

p: 'tar.gz',

Copy link

Member

paulmelnikow Dec 19, 2018

Choose a reason for hiding this comment

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

👍

paulmelnikow reviewed Dec 19, 2018

View reviewed changes

calebcartwright and others added 4 commits December 19, 2018 11:40

feat: updated nexus service

35144bf

Merge branch 'master' into master

28c27f5

feat: more nexus service cleanup

a9fc585

Merge branch 'master' of https://github.com/calebcartwright/shields

c9f38b2

paulmelnikow approved these changes Dec 19, 2018

View reviewed changes

paulmelnikow merged commit 6d3798f into badges:master Dec 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrates [Nexus] service to new service model #2520

Migrates [Nexus] service to new service model #2520

calebcartwright commented Dec 13, 2018 •

edited

Loading

shields-ci commented Dec 13, 2018 •

edited

Loading

calebcartwright commented Dec 18, 2018 •

edited

Loading

paulmelnikow left a comment

paulmelnikow Dec 18, 2018

paulmelnikow Dec 18, 2018

calebcartwright Dec 19, 2018

paulmelnikow Dec 19, 2018

paulmelnikow Dec 18, 2018

calebcartwright Dec 19, 2018

paulmelnikow Dec 19, 2018

calebcartwright Dec 19, 2018

paulmelnikow Dec 18, 2018

calebcartwright Dec 19, 2018

calebcartwright Dec 19, 2018

paulmelnikow Dec 19, 2018

paulmelnikow Dec 19, 2018

calebcartwright Dec 19, 2018

paulmelnikow Dec 19, 2018

calebcartwright Dec 19, 2018

calebcartwright commented Dec 19, 2018

paulmelnikow Dec 19, 2018

paulmelnikow Dec 19, 2018 •

edited

Loading

paulmelnikow Dec 19, 2018

paulmelnikow left a comment

calebcartwright commented Dec 19, 2018

shields-deployment bot commented Dec 19, 2018

paulmelnikow commented Dec 19, 2018

Migrates [Nexus] service to new service model #2520

Migrates [Nexus] service to new service model #2520

Conversation

calebcartwright commented Dec 13, 2018 • edited Loading

shields-ci commented Dec 13, 2018 • edited Loading

calebcartwright commented Dec 18, 2018 • edited Loading

paulmelnikow left a 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

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

calebcartwright commented Dec 19, 2018

Choose a reason for hiding this comment

paulmelnikow Dec 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmelnikow left a comment

Choose a reason for hiding this comment

calebcartwright commented Dec 19, 2018

shields-deployment bot commented Dec 19, 2018

paulmelnikow commented Dec 19, 2018

calebcartwright commented Dec 13, 2018 •

edited

Loading

shields-ci commented Dec 13, 2018 •

edited

Loading

calebcartwright commented Dec 18, 2018 •

edited

Loading

paulmelnikow Dec 19, 2018 •

edited

Loading