Migrate [Jira] services to new service model #2541

calebcartwright · 2018-12-16T18:48:32Z

Migrates the Jira badge services to the new service model, and also adds tests for those badges.

I did make one minor changes to the badge labels/messages when the issue or sprint cannot be found. For example:

With the LegacyService implementation, when an issue was not found the badge would display as 'issue key | inaccessible' such as KAFKA-2869 | inaccessible but this changes that behavior to jira | issue not found. I personally find this to be a bit more informative of a message, but want to call it out as I know how important backwards compatibility is. However, at first glance I'm also not sure how we'd replicate the old behavior of using the issue key (provided by user as a param) as the badge label when an issue is not found because the API response has a status code of 404 and a different schema than what Joi is expecting/validating.

Similarly for the sprints, the old format for a sprint not found was completion | inaccessible it's now jira | sprint not found

shields-ci · 2018-12-16T18:49:17Z

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

Generated by 🚫 dangerJS against 070dec5

calebcartwright · 2018-12-16T19:03:18Z

Also is there a pattern when writing service tests to cover lines that are using auth info from server-secrets?

services/jira/jira-issue.tester.js

paulmelnikow · 2018-12-17T22:06:28Z

Also is there a pattern when writing service tests to cover lines that are using auth info from server-secrets?

No… though it would be possible!

You could use an IcedFrisby before() hook in a service test, like the one @PyvesB just added here:

shields/services/wheelmap/wheelmap.tester.js

Line 18 in b4b29ab

.before(logTokenWarning)

The cleanup would go in an IcedFrisby .finally() hook. Probably most reliable using sinon.

You could generalize the code here into a mockShieldsSecrets() function that creates and returns a sinon sandbox.

shields/services/github/auth/admin.spec.js

Lines 24 to 35 in b4b29ab

    
           let sandbox 
        
           beforeEach(function() { 
        
             sandbox = sinon.createSandbox() 
        
             // Make this work when there is no `shieldsSecret` defined. 
        
             serverSecrets.shieldsSecret = undefined 
        
             sandbox 
        
               .stub(serverSecrets, 'shieldsSecret') 
        
               .value(validCredentials.password) 
        
           }) 
        
           afterEach(function() { 
        
             sandbox.restore() 
        
           })

paulmelnikow

This looks awesome! Thanks for the tests! I left you a few minor comments.

services/jira/jira-base.js

paulmelnikow · 2018-12-17T22:12:19Z

services/jira/jira-issue.service.js

  }

  static get route() {
    return {
      base: 'jira/issue',
+      pattern: ':protocol(https?)/:host/:path?/:issueKey',


The regex in this pattern confused me for a moment. :protocol(http|https) seems clearer.

I think you need :path* to allow multiple path segments.

👍 on protocol

path is where I kept hitting a a mental wall with the pattern. I was trying to use nested non-capture groups but pattern was having none of it 😆 Let me see if I can clean that up a bit

I ended up using a specific regex ((.+)) for the host param, which works and is more consistent with the legacy service info (which did not explicitly differentiate host and path)

I'm fine with the change you made, though I think hostAndPath would better describe the field for the user (this is with #2496 in mind).

yeah I like that better too (and that's what was there before so +1 for consistency!)

changed to hostAndPath in both the example pattern, route pattern, and param to handle for consistency. I elected not to include the + suffix but let me know if it is needed

(original comment) i'm probably being overly pedantic here, but just noticed the example pattern before was actually hostAndPath+ does the + suffix have any significance in the example patterns/is it worth including?

In a pattern, the + tells pathToRegexp to allow one or more segments instead of just one, zero or one, or zero or more. We aren’t doing anything using the pattern in the examples beyond parsing it to get the individual segments. (That’s forthcoming actually, in #2496)

services/jira/jira-issue.service.js

services/jira/jira-issue.tester.js

services/jira/jira-sprint.service.js

calebcartwright · 2018-12-17T23:06:24Z

services/jira/jira-sprint.service.js

+    url += '/rest/api/2/search'
+    const options = {
+      qs: {
+        jql: `sprint=${sprintId} AND type IN (Bug,Improvement,Story,"Technical task")`,


I added the above comment on url to indicate the usage of search over sprint API, and I totally get why it is implemented that way. However, I could envision some potential issues due the issue type clause that we have to put into the jql query. Jira is used by some teams at my day job, and we've created many custom issue types (like 12387123897612387123 of them 😄) that wouldn't be accurately reflected by this query.

IMO we shouldn't try to solve that in this PR, but it may be worth revisiting at some point

calebcartwright · 2018-12-18T00:31:47Z

Thanks for the feedback! I think I've addressed everything, and also added a test helpers file to help facilitate the setup/restore to validate auth scenarios

paulmelnikow · 2018-12-18T04:02:17Z

services/jira/jira-test-helpers.js

+}
+
+function restore() {
+  sinon.restore()


It looks like this function is not intended to be used publicly: sinonjs/sinon#1390

Since [email protected] the default object is itself a sandbox, and most of the documentation says to use that directly for the majority of use cases, https://sinonjs.org/releases/v7.2.2/sandbox/.

They show this pattern, including the restore function on the docs for migrating to [email protected] https://sinonjs.org/guides/migrating-to-5.0.html.

In that issue I see references to [email protected]/4.x so maybe the usage of restore has changed?

paulmelnikow · 2018-12-18T04:03:20Z

services/jira/jira-issue.tester.js

+  .intercept(nock =>
+    nock('https://myprivatejira.com/rest/api/2/issue')
+      .get(`/${encodeURIComponent('secure-234')}`)
+      .basicAuth({


Since this pattern is a bit unfamiliar, you could comment here that by including this here, it ensures the credentials are actually being sent out with the HTTP request. Otherwise the request wouldn't match and the test would fail.

Nice work on this test, by the way!

shields-deployment · 2018-12-18T04:30:36Z

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-18T21:41:56Z

btw @calebcartwright I wanted to reach out and see about setting up a time to say hi. Could you shoot a quick message? The address on my profile works.

calebcartwright · 2018-12-18T21:59:24Z

will do!

Augmenting the tests for Jira that were added in #2541 to also validate color scheme

feat: refactor Jira services to new service model

ad30bcf

calebcartwright commented Dec 16, 2018

View reviewed changes

services/jira/jira-issue.tester.js Show resolved Hide resolved

calebcartwright mentioned this pull request Dec 16, 2018

The Great Service Rewrite 🏆 #1358

Closed

paulmelnikow added the service-badge New or updated service badge label Dec 16, 2018

paulmelnikow reviewed Dec 17, 2018

View reviewed changes

Merge branch 'master' into jira

65f78cf

calebcartwright commented Dec 17, 2018

View reviewed changes

feat: updated jira services based on feedback

b640b5d

paulmelnikow reviewed Dec 18, 2018

View reviewed changes

calebcartwright and others added 2 commits December 17, 2018 22:27

feat: updates to jira services and tests

9fb8817

Merge branch 'master' into jira

070dec5

paulmelnikow approved these changes Dec 18, 2018

View reviewed changes

paulmelnikow merged commit 7d30d86 into badges:master Dec 18, 2018

calebcartwright deleted the jira branch December 18, 2018 04:34

calebcartwright mentioned this pull request Dec 19, 2018

Update [Jira] tests to include color validation #2565

Merged

paulmelnikow pushed a commit that referenced this pull request Dec 20, 2018

Update [Jira] tests to include color validation (#2565)

fc41b57

Augmenting the tests for Jira that were added in #2541 to also validate color scheme

snyk-bot mentioned this pull request Sep 1, 2021

[Snyk] Security upgrade request from 2.75.0 to 2.82.0 mkg20001/shields#13

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate [Jira] services to new service model #2541

Migrate [Jira] services to new service model #2541

calebcartwright commented Dec 16, 2018 •

edited

Loading

shields-ci commented Dec 16, 2018 •

edited

Loading

calebcartwright commented Dec 16, 2018

paulmelnikow commented Dec 17, 2018

paulmelnikow left a comment

paulmelnikow Dec 17, 2018

calebcartwright Dec 17, 2018

calebcartwright Dec 18, 2018

paulmelnikow Dec 18, 2018

calebcartwright Dec 18, 2018

calebcartwright Dec 18, 2018 •

edited

Loading

paulmelnikow Dec 18, 2018

calebcartwright Dec 17, 2018

calebcartwright commented Dec 18, 2018

paulmelnikow Dec 18, 2018

calebcartwright Dec 18, 2018 •

edited

Loading

paulmelnikow Dec 18, 2018

paulmelnikow Dec 18, 2018

shields-deployment bot commented Dec 18, 2018

paulmelnikow commented Dec 18, 2018

calebcartwright commented Dec 18, 2018

Migrate [Jira] services to new service model #2541

Migrate [Jira] services to new service model #2541

Conversation

calebcartwright commented Dec 16, 2018 • edited Loading

shields-ci commented Dec 16, 2018 • edited Loading

calebcartwright commented Dec 16, 2018

paulmelnikow commented Dec 17, 2018

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

calebcartwright Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcartwright commented Dec 18, 2018

Choose a reason for hiding this comment

calebcartwright Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shields-deployment bot commented Dec 18, 2018

paulmelnikow commented Dec 18, 2018

calebcartwright commented Dec 18, 2018

calebcartwright commented Dec 16, 2018 •

edited

Loading

shields-ci commented Dec 16, 2018 •

edited

Loading

calebcartwright Dec 18, 2018 •

edited

Loading

calebcartwright Dec 18, 2018 •

edited

Loading